Feature: implement snowflake export function #156 by onlyjackfrost · Pull Request #163 · Canner/vulcan-sql

onlyjackfrost · 2023-04-24T10:30:48Z

Description

This pull request implemented the snowflake export function to export the selected data to the local dir in parquet format.

export flow:

Use the snowflake "COPY INTO..." command to convert the selected data into parquet format and stored in the user stage. This request will have a UUID generated by Snowflake, we will append this unique id to the stage file name to prevent concurrent copy requests with the same name which Snowflake will throw an error.
Use the snowflake "GET ..." command to download the parquet file stored in the user stage to the local directory. We Use the "pattern" parameter in this command and filter out the file with the unique id.
Use the snowflake "Remove ..." command to remove the parquet file in the user stage to avoid increasing costs. We Use the "pattern" parameter in this command and filter out the file with the unique id.

Issue ticket number

issue #156

Additional Context

The encoding type of columns in the parquet file seems can not be specified in Snowflake.
We add a query Id (a uuid generated by snowflake) in our stage file name and allow partition when reach the file size limit
We set the max file size of each file to the maximum of snowflake setting (5GB)
The max file size does not have a lower bound, but the minimum file size is 1 Mb though. (found when developing)
the partition file will have a subfix in the file name like "\d{1,2}\d{1,2}_\d{1,2}.parquet"

The COPY command does not validate data type conversions for Parquet files. see Usage Note.
The GET command will auto-decrypt the parquet file with the key that is used when encrypting., see Usage Note
Still can not get the proper encoded type in column after adjusting the session parameter ENABLE_UNLOAD_PHYSICAL_TYPE_OPTIMIZATION , see Note(May encounter invalid parquet type when reading parquet file using parquetjs or derivative package like parquets)

Result

Use parquet-cli to view the downloaded parquet file

vercel · 2023-04-24T10:30:52Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
vulcan-sql-document	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 4, 2023 2:43am

codecov-commenter · 2023-05-02T17:22:35Z

Codecov Report

Patch coverage: 92.68% and project coverage change: +0.02 🎉

Comparison is base (7a79c10) 92.91% compared to head (dc72b97) 92.94%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #163      +/-   ##
===========================================
+ Coverage    92.91%   92.94%   +0.02%     
===========================================
  Files          291      291              
  Lines         4473     4506      +33     
  Branches       604      606       +2     
===========================================
+ Hits          4156     4188      +32     
- Misses         203      204       +1     
  Partials       114      114

Flag	Coverage Δ
build	`94.64% <ø> (ø)`
core	`93.96% <0.00%> (-0.05%)`	⬇️
extension-dbt	`97.43% <ø> (ø)`
extension-debug-tools	`98.11% <ø> (ø)`
extension-driver-bq	`85.21% <ø> (ø)`
extension-driver-duckdb	`97.53% <ø> (ø)`
extension-driver-pg	`96.11% <ø> (ø)`
extension-driver-snowflake	`96.22% <95.00%> (+1.63%)`	⬆️
integration-testing	`96.15% <ø> (ø)`
serve	`90.04% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/core/src/models/extensions/dataSource.ts	`87.50% <0.00%> (-5.84%)`	⬇️
...on-driver-snowflake/src/lib/snowflakeDataSource.ts	`94.59% <95.00%> (+4.11%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

kokokuo

Besides some suggestions, others LGTM 👍

kokokuo · 2023-04-25T07:03:41Z

packages/extension-driver-snowflake/src/lib/snowflakeDataSource.ts

-    this.logger.debug(`Acquiring connection from ${profileName}`);
-    const client = await pool.acquire();
-    this.logger.debug(`Acquired connection from ${profileName}`);
+    const { connection, pool } = await this.getConnection(profileName);


Great to refactor it ! 👍

kokokuo · 2023-05-03T06:32:01Z

packages/extension-driver-snowflake/src/lib/snowflakeDataSource.ts

+
+  // use protected for testing purpose
+  protected getCopyToStageSQL(sql: string, stageFilePath: string) {
+    return `COPY INTO ${stageFilePath} FROM (${sql}) FILE_FORMAT = (TYPE = 'parquet') HEADER=true INCLUDE_QUERY_ID=true MAX_FILE_SIZE=5368709120;`;


Suggest to add the comment to explain why to set the MAX_FILE_SIZE is 5368709120 not other values, and paste the document link to help other members know :D

kokokuo · 2023-05-03T07:13:00Z

packages/extension-driver-snowflake/src/lib/snowflakeDataSource.ts

+    const { profileName, sql } = exportOptions;
+
+    const { connection, pool } = await this.getConnection(profileName);
+    directory = /^\./.exec(directory) ? directory : `./${directory}`;


Currently, our directory may be a relative path based on the project or an absolute path temp directory from the root if the user does not set folderPath in the cache settings of the vulcan.yaml:

Relative path: > `tmp/schem1/pg/cache_department` Absolute path: > `/var/folders/51/049hk_157kgc8ht18q61c1lm0000gn/T/vulcan/cache/schem1/pg/cache_department`

If the snowflake GET path needs the absolute path, then currently the checking directory has . before the path may cause an issue, and need to remove it.

We could use path.resolve to normalize the above two cases directly, before is the screenshot (seems you have written it in the following code):

Btw, suggest adding the comment to explain the reason we need to get the absolute path from the Snowflake GET method needed and paste the document link to help other members know it.

kokokuo · 2023-05-03T07:14:45Z

packages/extension-driver-snowflake/src/lib/snowflakeDataSource.ts

+      const copyStatement = await this.getStatementFromSQL(
+        connection,
+        builtSQL,
+        []


Maybe we could refactor getStatementFromSQL methods to make the binding arguments could be [] array default, then we don't need always to pass [] when copying to stage, getting files, and removing stage.

kokokuo · 2023-05-03T07:22:23Z

packages/extension-driver-snowflake/src/lib/snowflakeDataSource.ts

+  public override async export(exportOptions: ExportOptions): Promise<void> {
+    let { directory } = exportOptions;
+    const { profileName, sql } = exportOptions;
+


Suggest removing the newline.

kokokuo · 2023-05-03T08:29:20Z

packages/extension-driver-snowflake/test/snowflakeDataSource.spec.ts

+  // Assert
+  const files = fs.readdirSync(directory);
+  expect(files.length).toBe(1);
+  expect(/parquet$/.exec(path.basename(files[0])));


Well, the expect(/parquet$/.exec(path.basename(files[0]))) actually may not really assert the result.

The value in the expect(value) is the actual value, we should call the method to do the assert.

Please use the expect method to assert the result, e.g:

expect(files[0]).toMatch('/.parquet$/');

kokokuo · 2023-05-03T08:35:17Z

packages/extension-driver-snowflake/test/snowflakeDataSource.spec.ts

+  const files = fs.readdirSync(directory);
+  expect(files.length).toBe(4);
+  // check each file in the files exists
+  files.forEach((file) => {


The same issue and suggestion line above.

kokokuo · 2023-05-03T08:38:00Z

packages/extension-driver-snowflake/test/snowflakeDataSource.spec.ts

Suggest to enhance test cases, according to the export method logistics, we may still need two test cases, first is checking the directory not exist and throw error, second is the checking the happened cached error case

kokokuo · 2023-05-03T08:39:33Z

packages/extension-driver-snowflake/src/lib/snowflakeDataSource.ts

+      const stage = '@~/vulcan_cache_stage';
+      const stageFilePath = `${stage}/`;
+      const builtSQL = this.getCopyToStageSQL(sql, stageFilePath);
+      // copy data to a named stage


Suggest to rich the comment to explain why we need to copy to the stage to help members know moving to the stage is necessary.

kokokuo · 2023-05-03T08:43:42Z

packages/extension-driver-snowflake/src/lib/snowflakeDataSource.ts

+      await this.waitStatement(getStatement);
+      this.logger.debug(`Exported parquet files to ${directory}`);
+
+      // remove parquet file from stage


Suggest rich the comment to describe removing the stage parquet files because we keep them in the stage temporarily for downloading, so it need to remove 😄

…rquet file to local

kokokuo

Thanks for fixing and update the commits, LGTM 👍 👍

vercel bot deployed to Preview April 24, 2023 10:37 View deployment

vercel bot deployed to Preview April 24, 2023 10:41 View deployment

vercel bot deployed to Preview April 24, 2023 15:44 View deployment

kokokuo self-requested a review April 25, 2023 06:36

vercel bot deployed to Preview May 2, 2023 07:00 View deployment

vercel bot deployed to Preview May 2, 2023 17:10 View deployment

kokokuo reviewed May 3, 2023

View reviewed changes

onlyjackfrost force-pushed the feature/snowflake-export-parquet branch from 588711c to ae0b2a0 Compare May 3, 2023 17:10

vercel bot deployed to Preview May 3, 2023 17:10 View deployment

onlyjackfrost added 2 commits May 4, 2023 10:39

feat(extension-driver-snowflake): export snowflake query result as pa…

847b3fe

…rquet file to local

chore: replace word arrange with assert

dc72b97

onlyjackfrost force-pushed the feature/snowflake-export-parquet branch from ae0b2a0 to dc72b97 Compare May 4, 2023 02:43

vercel bot deployed to Preview May 4, 2023 02:43 View deployment

kokokuo approved these changes May 4, 2023

View reviewed changes

kokokuo merged commit a75172c into develop May 4, 2023

kokokuo deleted the feature/snowflake-export-parquet branch May 4, 2023 04:23

Conversation

onlyjackfrost commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue ticket number

Additional Context

Result

Uh oh!

vercel bot commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kokokuo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokokuo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

onlyjackfrost commented Apr 24, 2023 •

edited

Loading

vercel bot commented Apr 24, 2023 •

edited

Loading

codecov-commenter commented May 2, 2023 •

edited

Loading