eric-maynard commented on code in PR #1378:
URL: https://github.com/apache/polaris/pull/1378#discussion_r2049991525
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1328,6 +1346,41 @@ 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
+ // currentMetadata / currentMetadataLocation
Review Comment:
What you're describing closely resembles a previous iteration of this fix,
which is validating. For reference I have exhumed it from my `git reflog`, see
here:
https://github.com/apache/polaris/compare/main...eric-maynard:polaris:less-refresh-retro
This is maybe even more convoluted than what you're describing, but does
this more or less make match what you're looking for?
The unfortunate thing is that this doesn't work, it fails the new test as
written. I didn't debug too much further as @singhpk234 convinced me that this
new approach is cleaner, but from what I remember the issue is that
`TableMetadata.buildFrom(...)` does not really work the way you'd like it to.
The invocation you shared fails with something like:
```
Cannot set metadata location with changes to table metadata: 2 changes
```
As @RussellSpitzer put it to me when we were discussing this, `Our builders
are too "smart"` :)
Long-term, I would like to move away from using
`BaseMetastoreTableOperations` altogether and then we can be a bit more free in
how we handle the metadata. We may still need to contribute changes to
TableMetadata upstream.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
