-
Notifications
You must be signed in to change notification settings - Fork 130
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
Upgrade Iceberg 1.7.0 #442
base: main
Are you sure you want to change the base?
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,6 @@ | |
import org.apache.iceberg.TableMetadata; | ||
import org.apache.iceberg.TableMetadataParser; | ||
import org.apache.iceberg.TableOperations; | ||
import org.apache.iceberg.aws.s3.S3FileIOProperties; | ||
import org.apache.iceberg.catalog.Namespace; | ||
import org.apache.iceberg.catalog.SupportsNamespaces; | ||
import org.apache.iceberg.catalog.TableIdentifier; | ||
|
@@ -99,7 +98,6 @@ | |
import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; | ||
import org.apache.polaris.core.storage.PolarisStorageIntegration; | ||
import org.apache.polaris.core.storage.StorageLocation; | ||
import org.apache.polaris.core.storage.aws.PolarisS3FileIOClientFactory; | ||
import org.apache.polaris.service.catalog.io.FileIOFactory; | ||
import org.apache.polaris.service.exception.IcebergExceptionMapper; | ||
import org.apache.polaris.service.task.TaskExecutor; | ||
|
@@ -2055,8 +2053,6 @@ private List<TableIdentifier> listTableLike(PolarisEntitySubType subType, Namesp | |
*/ | ||
private FileIO loadFileIO(String ioImpl, Map<String, String> properties) { | ||
Map<String, String> propertiesWithS3CustomizedClientFactory = new HashMap<>(properties); | ||
propertiesWithS3CustomizedClientFactory.put( | ||
S3FileIOProperties.CLIENT_FACTORY, PolarisS3FileIOClientFactory.class.getName()); | ||
Comment on lines
-2058
to
-2059
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No longer required because of this : https://github.com/apache/iceberg/pull/11259/files There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might have to mark this deprecated first since it's a public class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it was only ever used for this purpose, so we're probably fine |
||
return fileIOFactory.loadFileIO(ioImpl, propertiesWithS3CustomizedClientFactory); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound() throws IOException { | |
sessionCatalog.listNamespaces( | ||
sessionContext, Namespace.of("top_level", "whoops"))) | ||
.isInstanceOf(NoSuchNamespaceException.class) | ||
.hasMessage("Namespace does not exist: top_level.whoops"); | ||
.hasMessage("Namespace does not exist: top_level%1Fwhoops"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checking this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. found this change : apache/iceberg@5fc1413 looks like we changes the seperator, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no more intent to make the separator configurable. At least, that's the latest I know. There was a big discussion around this topic in August this year. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the response in the error message now? I think the dot in the error is much more user friendly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is interesting, as Iceberg still uses dot to join different level of namespaces, https://github.com/apache/iceberg/blob/09634857e4a1333f5dc742d1dca3921e9a9f62dd/api/src/main/java/org/apache/iceberg/catalog/Namespace.java#L97-L97. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops looks like it was a style miss : https://github.com/apache/polaris/actions/runs/11859822454/job/33055322598 error : There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems the revert PR (apache/iceberg#11574) is targeting Iceberg 1.7.1. Should we wait for the 1.7.1 release to avoid unnecessary back-and-forth? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, I think it's fair to wait as we not are chasing a release time in Polaris and 1.7.1 should be fast, I saw an RC is cut already apache/iceberg#11593 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me see, when the pr get merged and we have nightly artifact published, I can try reverting my change to correct the test and run our polaris suite with that ? wdyt There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can confirm its passing now, I have pointed to 1.8.0 artifact (can't find 1.7.1 in nightly) from nigthly, will change this 1.7.1 when it get released |
||
} | ||
} | ||
|
||
|
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.
Question: Is it a new transitive dependency brought by iceberg 1.7?
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.
yup, it's added as part of AWS S3InputStream retry handling via : https://github.com/apache/iceberg/pull/10433/files