Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 🎸 add bigquery option of with connection #1881

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hiracky16
Copy link
Contributor

@hiracky16 hiracky16 commented Dec 15, 2024

Overview

This PR adds support for the WITH CONNECTION option in BigQuery's CREATE TABLE statements.
https://cloud.google.com/bigquery/docs/reference/standard-sql/data-definition-language#create_table_statement

Key Changes

  1. New Option in table config
  • Added a withConnection option under the bigquery configuration.
  • This allows specifying the WITH CONNECTION option when executing CREATE TABLE statements in BigQuery.
  1. Background
  • This change is introduced to support Iceberg tables, which recently entered the pre-GA phase. The WITH CONNECTION option is required for certain use cases involving Iceberg tables.

Points to Review

  • Please confirm if the addition of the withConnection option in table config is appropriate.
  • Verify that this change does not affect other BigQuery options or processes.

I’d appreciate it if you could take a look at this when you have time. Thank you!

@@ -114,6 +119,7 @@ const IBigQueryOptionsProperties = () =>
"labels",
"partitionExpirationDays",
"requirePartitionFilter",
"withConnection",
Copy link
Contributor Author

@hiracky16 hiracky16 Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Ekrekr !
I am trying to add an option to the BigQuery config.
I added a new value to the config, but I am encountering a compilation error.
I am not sure what is causing it. Could you help me figure it out?

For reference, the following commands were successful:

bazel build cli  
bazel test //core/...  

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not your fault - we have a couple issues with our CI/CD which we're resolving currently.

I'll get this PR in once we've managed to fix them!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification! I understand the issue is on the CI/CD side.

Please let me know if there's anything I can do to help in the meantime. I’ll wait for the fix and look forward to the PR being merged.

Thanks again for your support!

@hiracky16 hiracky16 marked this pull request as ready for review December 16, 2024 01:48
@hiracky16 hiracky16 requested a review from a team as a code owner December 16, 2024 01:48
@hiracky16 hiracky16 requested review from Ekrekr and removed request for a team December 16, 2024 01:48
Copy link
Contributor

@Ekrekr Ekrekr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really well put together PR, and a very helpful feature, thank you so much!

cli/api/dbadapters/execution_sql.ts Outdated Show resolved Hide resolved
@@ -114,6 +119,7 @@ const IBigQueryOptionsProperties = () =>
"labels",
"partitionExpirationDays",
"requirePartitionFilter",
"withConnection",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not your fault - we have a couple issues with our CI/CD which we're resolving currently.

I'll get this PR in once we've managed to fix them!

@Ekrekr
Copy link
Contributor

Ekrekr commented Dec 27, 2024

I'm running the tests locally as a workaround to get this in while our CI failure is running - current error is

tests/api/api.spec.ts:1352:1 - error TS1128: Declaration or statement expected.

1352 });
     ~
tests/api/api.spec.ts:1352:2 - error TS1128: Declaration or statement expected.

1352 });

Plus needs a pull and sync, but then I expect that we'll be good to go!

@hiracky16
Copy link
Contributor Author

@Ekrekr
I have successfully passed all tests locally.
Thank you for improving test environment!

(!) Circular dependencies
node_modules/df/core/actions/index.d.ts -> node_modules/df/core/actions/assertion.d.ts -> node_modules/df/core/actions/index.d.ts
node_modules/df/core/session.d.ts -> node_modules/df/core/actions/index.d.ts -> node_modules/df/core/actions/assertion.d.ts -> node_modules/df/core/session.d.ts
node_modules/df/core/actions/index.d.ts -> node_modules/df/core/actions/data_preparation.d.ts -> node_modules/df/core/actions/index.d.ts
...and 15 more
created bazel-out/darwin-py2-fastbuild/bin/packages/@dataform/core/bundle.d.ts in 979ms
INFO: From Action packages/@dataform/core/bundle_no_license.js:
Hash: 6f46e5f303f26a12fa6c
Version: webpack 4.47.0
Time: 5461ms
Built at: 12/28/2024 5:13:03 PM
               Asset     Size  Chunks             Chunk Names
bundle_no_license.js  498 KiB       0  [emitted]  main
Entrypoint main = bundle_no_license.js
 [1] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/protos/ts.js 1.02 MiB {0} [built]
 [2] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/core/utils.js 39.5 KiB {0} [built]
 [3] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/common/protos/index.js 14.7 KiB {0} [built]
 [4] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/core/path.js 8.11 KiB {0} [built]
[23] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/core/actions/data_preparation.js 19.6 KiB {0} [built]
[24] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/core/actions/table.js 85.5 KiB {0} [built]
[25] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/core/actions/view.js 54.8 KiB {0} [built]
[26] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/core/version.js 430 bytes {0} [built]
[27] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/common/strings/stringifier.js 20.4 KiB {0} [built]
[28] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/core/compilers.js 28 KiB {0} [built]
[46] multi ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/packages/@dataform/core/index 28 bytes {0} [built]
[47] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/packages/@dataform/core/index.js 942 bytes {0} [built]
[48] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/core/index.js 3.52 KiB {0} [built]
[51] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/core/main.js 24.6 KiB {0} [built]
[72] ./bazel-out/host/bin/packages/@dataform/core/bundler.sh.runfiles/df/core/session.js 100 KiB {0} [built]
    + 61 hidden modules
INFO: From Executing genrule //packages/@dataform/core:package_tar:
a package
a package/configs.proto
a package/core.proto
a package/bundle.js
a package/bundle.d.ts
a package/package.json
INFO: Elapsed time: 48.982s, Critical Path: 47.66s
INFO: 17 processes: 2 internal, 8 darwin-sandbox, 7 worker.
INFO: Build completed successfully, 17 total actions
//core:main_test                                                         PASSED in 28.5s

Executed 1 out of 1 test: 1 test passes.
INFO: Build completed successfully, 17 total actions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants