-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 Delta INSERT when schema OR table location ends with two slashes #17980
Conversation
Configure bucket used in `TestIcebergS3AndGlueMetastoreTest` same way as in other `BaseS3AndGlueMetastoreTest` subclasses. Makes it easier to run tests locally. This also changes `TestIcebergGlueCatalogConnectorSmokeTest` for consistency.
This may be needed when working with MinIO containers where bucket names can be reused. This is not needed when working with real S3 where bucket names are unique. Also, it's unclear whether this is a safe operation when tests execute in parallel.
The `String.contains`-based inclusions/exclusions doesn't allow distinguishing between hypothetical cases like "trailing_slash" and "two_trailing_slashes". This also simplifies generated test locations for schemas. Previously the schema name was formatted twice into the location string.
Remove constructor which doesn't construct the table. Since it was randomizing the name, it couldn't even be used for dropping existing table.
Use try with resources to ensure that test tables are dropped even if an assertion fails.
ea91f94
to
29963d1
Compare
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.
Looks good except for CI failures.
// Hive normalizes double slash | ||
.replaceAll("(?<!(s3:))/+", "/"); |
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.
nit: double slash → multiple slash?
Build is 🔴
|
29963d1
to
ab96466
Compare
fixed iceberg test failures, but didn't fix delta yet |
ab96466
to
b6c9cf0
Compare
CI |
Extracted from #17958
For #17966, but does not fix it fully.