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

Switch to Metabase v1.48.3 #120

Merged
merged 1 commit into from
Feb 16, 2024
Merged

Switch to Metabase v1.48.3 #120

merged 1 commit into from
Feb 16, 2024

Conversation

lpoulain
Copy link
Collaborator

@lpoulain lpoulain commented Jan 24, 2024

Switch to Metabase v1.48.3. The following changes were made, as many new Metabase tests were added:

  • have-select-privilege? was replaced with sql-jdbc.sync.interface/have-select-privilege? as some tests could not find it. The test was also modified as the previous version didn't execute the actual query
  • Replaced the use of toucan.db (deprecated) with toucan2.core
  • Starburst actually does not support :foreign-keys
  • Two new tests were added which check whether sync works after a database was destroyed (a DBMS database, not a Metabase database). After many attempts to get around the issue, the only I found was to copy a modified version of driver_test.clj which removes those two tests which are not relevant to Starburst

@lpoulain lpoulain force-pushed the upgrade-to-metabase-149 branch 2 times, most recently from 94bdfa4 to a7cad00 Compare February 13, 2024 22:52
@andrewdibiasio6
Copy link
Member

👀

Copy link
Member

@andrewdibiasio6 andrewdibiasio6 left a comment

Choose a reason for hiding this comment

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

LGTM, great work. Had one small comment.

@@ -33,7 +33,7 @@
:native-parameters true
:expression-aggregations true
:binning true
:foreign-keys true
:foreign-keys false
Copy link
Member

Choose a reason for hiding this comment

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

Is there a doc for this for my reference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No Metabase doc. I realized this when I found that all the tests checking foreign keys failed. And it turns out that, when using the Trino JDBC driver, DatabaseMetaData.getImportedKeys() doesn't appear to work.

Copy link
Member

Choose a reason for hiding this comment

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

I meant do that says Trino does not support Foreign Keys. I just can't find it in the official docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One thing I'm researching in parallel is how come Presto Metabase driver supports foreign keys. It may be possible to have the Starburst driver support foreign keys, but it appears it never had.

Switch to Metabase v1.48.3
@lpoulain lpoulain merged commit 0eb1f76 into main Feb 16, 2024
2 checks passed
@lpoulain lpoulain linked an issue Feb 20, 2024 that may be closed by this pull request
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.

Upgrade to Metabase 1.48
2 participants