eric-maynard commented on code in PR #1378:
URL: https://github.com/apache/polaris/pull/1378#discussion_r2051404974
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1328,6 +1344,25 @@ public void doCommit(TableMetadata base, TableMetadata
metadata) {
String newLocation = writeNewMetadataIfRequired(base == null, metadata);
String oldLocation = base == null ? null : base.metadataFileLocation();
+ // TODO: we should not need to do this hack, but there's no other way to
modify
+ // metadataFileLocation / currentMetadataLocation
+ if (updateMetadataOnCommit) {
+ try {
+ Field tableMetadataField =
TableMetadata.class.getDeclaredField("metadataFileLocation");
+ tableMetadataField.setAccessible(true);
+ tableMetadataField.set(metadata, newLocation);
Review Comment:
For some reason `IcebergCatalogHandler` is written to be semi-agnostic of
the underlying `Catalog` type and the code does not assume it's an
`IcebergCatalog` -- you can see this all over the place, where we do things
like:
```
this.viewCatalog = (baseCatalog instanceof ViewCatalog) ? (ViewCatalog)
baseCatalog : null;
```
We could add a case here where we check, similarly, if the underlying
catalog is an `IcebergCatalog` and then we can add a method to IcbergCatalog
for register/commit which goes through our own route and our own
TableOperations etc. As you note we'd also have to write our own `Table`, and
very quickly this starts to look like the refactor/rewrite discussed elsewhere.
tbh I'm happy to take that work on, but I wanted to see if we are OK with this
smaller patch in the interim.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
