Bigquery sync table require filter #36677

qnkhuat · 2023-12-12T10:55:20Z

First milestone of #36668

Known limitation: this won't work for materialized views, see https://github.com/metabase/metabase/pull/36677/files#diff-370f1464c7c10bd0d0bd84544e6f80102152617fcd99fea6e9bd4dbaff4903caR131-R135 for why

To read more about partitioned tables, see https://cloud.google.com/bigquery/docs/partitioned-tables

cypress · 2023-12-15T05:11:51Z

4 flaky tests on run #617 ↗︎

0	2179	159	0	4

Details:

Bigquery sync table require filter
Project: Metabase e2e	Commit: `14bff219ec`
Status: Passed	Duration: 15:13 💡
Started: Dec 20, 2023 8:26 AM	Ended: Dec 20, 2023 8:41 AM

sharing/subscriptions.cy.spec.js • 1 flaky test • sharing

View Output

Test		Artifacts
scenarios > dashboard > subscriptions > EE email subscriptions > should only show users in same group in recipients dropdown if `user-visiblity` setting is `group`		`Test Replay` `Screenshots`

sharing/downloads/reproductions/19889-native-query-export-column-order.cy.spec.js • 2 flaky tests • flaky

View Output

Test		Artifacts
issue 19889 > should order columns correctly in saved native query exports		`Test Replay` `Screenshots`
issue 19889 > should order columns correctly in saved native query exports when the query was modified but not re-run before save (#19889)		`Test Replay` `Screenshots`

question/notebook.cy.spec.js • 1 flaky test • slow

View Output

Test		Artifacts
scenarios > question > notebook > should process the updated expression when pressing Enter		`Test Replay` `Screenshots`

Review all test suite changes for PR #36677 ↗︎

qnkhuat · 2023-12-15T10:55:58Z

src/metabase/sync/sync_metadata/tables.clj

-         our-metadata            (our-metadata database)
-         strip-desc              (fn [metadata]
-                                   (set (map #(dissoc % :description) metadata)))
+         name+schema->db-table   (m/index-by (juxt :name :schema) db-tables)


I took a stab at refactoring this sync step a bit.
Details in the following comments

src/metabase/sync/sync_metadata/tables.clj

qnkhuat · 2023-12-15T11:01:18Z

src/metabase/sync/sync_metadata/tables.clj

-     (when (seq changed-tables)
-       (sync-util/with-error-handling (format "Error updating table description for %s" (sync-util/name-for-logging database))
-         (update-table-description! database changed-tables)))
+     (sync-util/with-error-handling (format "Error updating table metadata for %s" (sync-util/name-for-logging database))


previously, only table description could be updated during sync step, but I've changed it so that it could extend to other properties too.

qnkhuat · 2023-12-15T11:02:30Z

src/metabase/sync/sync_metadata/tables.clj

+                                     table-metadata)
+                                    to-update-keys)
+        [_ changes _]  (data/diff old-table new-table)
+        changes        (cond-> changes


this is a specical case, check test metabase.sync.sync-metadata.comments-test/sync-existing-table-comment-test and metabase.sync.sync-metadata.comments-test/dont-overwrite-table-custom-description-test to know how it's expected to work.

src/metabase/sync/sync_metadata/tables.clj

qnkhuat · 2023-12-15T11:14:02Z

test/metabase/sync/sync_metadata/comments_test.clj

@@ -114,7 +117,14 @@
      (mt/dataset (basic-table "table_with_comment_after_sync" nil)
        ;; modify the source DB to add the comment and resync
        (driver/notify-database-updated driver/*driver* (mt/db))
-        (tx/create-db! driver/*driver* (basic-table "table_with_comment_after_sync" "added comment"))
+        (tx/create-db! driver/*driver* (basic-table "table_with_comment_after_sync" nil))


This test relied on the fact that descriptions are not included when inserting tables, the correct behavior is to start with a table without a description, then sync, then add a description, then sync. see https://github.com/metabase/metabase/pull/36677/files#r1427842665 for context.

qnkhuat · 2023-12-15T11:17:15Z

test/metabase/automagic_dashboards/core_test.clj

@@ -185,8 +185,8 @@
  (mt/with-test-user :rasta
    (automagic-dashboards.test/with-dashboard-cleanup
      (doseq [[table cardinality] (map vector
-                                       (t2/select Table :db_id (mt/id) {:order-by [[:id :asc]]})
-                                       [15 11 7 5])]
+                                       (t2/select Table :db_id (mt/id) {:order-by [[:name :asc]]})


There are 2 things happening here:

this is a consequence of test tables ids being changed (context).

after Move sample-dataset into test-data for backend tests #35973, the test-data database now has 8 tables. so that's why you see my changes also include 4 more items.

qnkhuat · 2023-12-19T06:07:07Z

e2e/test/scenarios/admin/datamodel/hide_tables.cy.spec.js

@@ -16,15 +16,15 @@ describe("scenarios > admin > datamodel > hidden tables (metabase#9759)", () =>

  it("hidden table should not show up in various places in UI", () => {
    // Visit the main page, we shouldn't be able to see the table
-    cy.visit(`/browse/${PRODUCTS_ID}`);
+    cy.visit(`/browse/${SAMPLE_DB_ID}`);


this was a mistake and it accidentally works because producs_id and sample_db_id had the same id (=1)

🤦‍♂️

modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj

src/metabase/sync/sync_metadata/tables.clj

Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com>

modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj

calherries

Solid work

calherries

Actually sorry, I think you're missing a test case for a table that is initially created without a required filter, then synced, then is modified to have a required filter, then synced again. Similar to sync-existing-table-comment-test

qnkhuat · 2023-12-20T03:00:20Z

@calherries

I think you're missing a test case for a table that is initially created without a required filter, then synced, then is modified to have a required filter, then synced again. Similar to sync-existing-table-comment-test

This is not possible in bigquery because whether a table requires partition or not is a property it's created with. afaik You can't change it after.

I could add sth like sync-table-update-info-of-new-table-added-during-sync-test tho.

calherries · 2023-12-20T07:51:52Z

Based on the docs (here and here), it looks like you can update the property after the table is created.

calherries · 2023-12-20T07:53:23Z

Also using SQL (source):

ALTER TABLE IF EXISTS mydataset.newtable
SET OPTIONS(
    require_partition_filter = true
)

qnkhuat · 2023-12-20T08:14:21Z

Good find! I was looking at the list of options for ALTER TABLE SET OPTIONS command(docs) and didn't find this option so I thought it's not possible.

anw I added the test here 8452b0b

calherries

Great, thanks!

github-actions · 2023-12-20T09:40:19Z

@qnkhuat Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

metabase-bot bot assigned qnkhuat Dec 12, 2023

metabase-bot bot added the .Team/BackendComponents also known as BEC label Dec 12, 2023

synced successully

7352041

qnkhuat force-pushed the ngoc-bigquery-sync-filter-key branch from 869f11f to 7352041 Compare December 15, 2023 04:15

qnkhuat added 2 commits December 15, 2023 11:35

better naming

d412a8b

fix tests can't compile

a5c71d5

qnkhuat added 5 commits December 15, 2023 14:24

fix sync tests for h2 and postgres

af9efe4

fix tests, what's a weird test case

dba5f8d

add a tests and fix the view test

b716c3d

fix cypress tests can't sart

3086934

add sync table test

a828494

qnkhuat commented Dec 15, 2023

View reviewed changes

src/metabase/sync/sync_metadata/tables.clj Outdated Show resolved Hide resolved

qnkhuat commented Dec 15, 2023

View reviewed changes

src/metabase/sync/sync_metadata/tables.clj Outdated Show resolved Hide resolved

qnkhuat commented Dec 15, 2023

View reviewed changes

qnkhuat added 2 commits December 15, 2023 18:27

fix can't sync

6692779

fix test

b948ad1

qnkhuat mentioned this pull request Dec 18, 2023

[Epic] Bigquery: Sync required partition column #36668

Closed

qnkhuat added 6 commits December 18, 2023 10:36

should fix all the failing e2e tests?

7d6d94e

materialized views work too

0224d74

Merge branch 'master' into ngoc-bigquery-sync-filter-key

935fbbb

flip the predicate

3d84c17

not my migration id

c4402f0

make sure it's a boolean

58d83a3

qnkhuat requested a review from a team December 18, 2023 10:15

partitioned table that doesn't require filter

fb49dea

qnkhuat requested a review from calherries December 19, 2023 04:52

qnkhuat commented Dec 19, 2023

View reviewed changes

typo

6089aae

calherries reviewed Dec 19, 2023

View reviewed changes

modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj Show resolved Hide resolved

calherries reviewed Dec 19, 2023

View reviewed changes

src/metabase/sync/sync_metadata/tables.clj Outdated Show resolved Hide resolved

qnkhuat and others added 2 commits December 19, 2023 18:56

add note for materialized views and resolve Cal's comments

ce46551

Update src/metabase/sync/sync_metadata/tables.clj

ee5f722

Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com>

qnkhuat requested a review from calherries December 19, 2023 12:05

calherries reviewed Dec 19, 2023

View reviewed changes

modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj Outdated Show resolved Hide resolved

calherries approved these changes Dec 19, 2023

View reviewed changes

remove debug code

c5120bd

calherries requested changes Dec 19, 2023

View reviewed changes

nemanjaglumac mentioned this pull request Dec 19, 2023

FE unit tests are using hard-coded IDs #36954

Open

qnkhuat requested a review from calherries December 20, 2023 03:00

qnkhuat added 4 commits December 20, 2023 10:02

Merge branch 'master' into ngoc-bigquery-sync-filter-key

d9bc810

revert the id changes in FE preset sample dataset

e9b2123

Fix e2e tests

e0b7afb

fix reflection warnings

07a906e

add update require filter option test

8452b0b

calherries approved these changes Dec 20, 2023

View reviewed changes

qnkhuat added the no-backport Do not backport this PR to any branch label Dec 20, 2023

qnkhuat merged commit ede142b into master Dec 20, 2023
107 of 108 checks passed

qnkhuat deleted the ngoc-bigquery-sync-filter-key branch December 20, 2023 09:40

qnkhuat added this to the 0.49 milestone Dec 20, 2023

shaunlebron pushed a commit that referenced this pull request Dec 20, 2023

Bigquery sync table require filter (#36677)

5b5bfa4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bigquery sync table require filter #36677

Bigquery sync table require filter #36677

qnkhuat commented Dec 12, 2023 •

edited

Loading

cypress bot commented Dec 15, 2023 •

edited

Loading

qnkhuat Dec 15, 2023

qnkhuat Dec 15, 2023

qnkhuat Dec 15, 2023

qnkhuat Dec 15, 2023

qnkhuat Dec 15, 2023

qnkhuat Dec 19, 2023

nemanjaglumac Dec 19, 2023

calherries left a comment

calherries left a comment •

edited

Loading

qnkhuat commented Dec 20, 2023

calherries commented Dec 20, 2023

calherries commented Dec 20, 2023

qnkhuat commented Dec 20, 2023

calherries left a comment

github-actions bot commented Dec 20, 2023

Bigquery sync table require filter #36677

Bigquery sync table require filter #36677

Conversation

qnkhuat commented Dec 12, 2023 • edited Loading

cypress bot commented Dec 15, 2023 • edited Loading

4 flaky tests on run #617 ↗︎

Review all test suite changes for PR #36677 ↗︎

qnkhuat Dec 15, 2023

Choose a reason for hiding this comment

qnkhuat Dec 15, 2023

Choose a reason for hiding this comment

qnkhuat Dec 15, 2023

Choose a reason for hiding this comment

qnkhuat Dec 15, 2023

Choose a reason for hiding this comment

qnkhuat Dec 15, 2023

Choose a reason for hiding this comment

qnkhuat Dec 19, 2023

Choose a reason for hiding this comment

nemanjaglumac Dec 19, 2023

Choose a reason for hiding this comment

calherries left a comment

Choose a reason for hiding this comment

calherries left a comment • edited Loading

Choose a reason for hiding this comment

qnkhuat commented Dec 20, 2023

calherries commented Dec 20, 2023

calherries commented Dec 20, 2023

qnkhuat commented Dec 20, 2023

calherries left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2023

qnkhuat commented Dec 12, 2023 •

edited

Loading

cypress bot commented Dec 15, 2023 •

edited

Loading

calherries left a comment •

edited

Loading