-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Test iceberg_tables function with TrinoSnowflakeCatalog #24593
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
Test iceberg_tables function with TrinoSnowflakeCatalog #24593
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test is flaky. What happens if a new object is created after listing iceberg.information_schema.tables and before executing iceberg_tables function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the reason the generic test creates its own schemas but in this case, it is not needed because this test does not create iceberg tables other than potentially during setup.
There is one test that creates a native table but that will not be listed by TrinoSnowflakeCatalog#listTables
|
@ebyhr could you run with secrets to actually run this test? |
|
/test-with-secrets sha=181dc8c8651fdc68b83c9d3d520ef87e13a3f6b7 |
|
The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/12583104964 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to build VALUES query? Can we simplify like below?
String schemaName = SNOWFLAKE_TEST_SCHEMA.toLowerCase(ENGLISH);
assertThat(query("SELECT * FROM TABLE(iceberg.system.iceberg_tables(SCHEMA_NAME => '" + schemaName + "'))"))
.matches("SELECT table_schema, table_name FROM iceberg.information_schema.tables WHERE table_schema = '" + schemaName + "'");There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The QueryAssert is way better, let me try that
181dc8c to
ec70948
Compare
|
/test-with-secrets sha=ec70948da143156d369a3a195a9acb7fa134b65f |
|
The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/12648508425 |
|
Let me close this PR because we revert the function addition in #24719 |
Description
Implement a simple test for the
system.iceberg_tables tablefunction for theTrinoSnowflakeCatalog.Since
TrinoSnowflakeCatalogis read only the original generic test does not work.Theoretically, we could apply the modifications directly on the snowflake to match the generic test with multiple schemas. However, that seems like an overkill given the implementation just delegates to the
listTablesmethod and maps the results.Additional context and related issues
Release notes
( X) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: