-
Notifications
You must be signed in to change notification settings - Fork 57
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
Fix quoted column name for Iceberg primitive type #891
Conversation
name = "enableIcebergStreaming={0}, compressionAlgorithm={1}, icebergSerializationPolicy={2}") | ||
public static Object[][] parameters() { | ||
return new Object[][] { | ||
// TODO: Enable this after Iceberg testing is set up on sfctest0 GCP / Azure |
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.
With our plan to run two IT suites in parallel for iceberg/non-iceberg, how will this class be run in one mode in suite 1 and another mode in suite 2?
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.
Will split this into two separate tests in later PR.
src/test/java/net/snowflake/ingest/streaming/internal/it/ColumnNamesIT.java
Show resolved
Hide resolved
enableIcebergStreaming ? "iceberg" : "", | ||
tableName, | ||
createTableColumnName, | ||
enableIcebergStreaming ? getIcebergTableConfig(tableName) : ""); |
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.
IMO we should refactor this to have a method that takes in the table name and column schema list only, and adds the proper prefix (iceberg table vs. table) and proper suffix (catalog and external_volume and base_location).
Instead of every create table call having to worry about both these things.
If this PR is good to merge am ok with this test refactoring in the next PR. Please also pick up any deferred comments from the last PR if any!
This PR fix the Iceberg ITs accidentally broke by previous #891. No behavior change.
For non-structured data type column, server sides perform EP info check which requires the column display name to be exactly the same as the quoted column name. Fix a bug on this and extend existing column name test to run on iceberg as in preprod as well. Disable the Iceberg test now the pass the merge gate.