Core: Discard changes when suppressing historical snapshots in CatalogHandlers

[ebyhr]

• Fixes Core: Discard changes when suppressing historical snapshots in CatalogHandlers #17538
• Closes fix: discard changes when suppressing historical snapshots in CatalogHandlers #17539
  • The PR doesn't have any test and no response for a while.

[ebyhr]

This line throws an exception if we revert CatalogHandlers's change:

Cannot set metadata location with changes to table metadata: 1 changes
java.lang.IllegalArgumentException: Cannot set metadata location with changes to table metadata: 1 changes
	at org.apache.iceberg.rest.ErrorHandlers$DefaultErrorHandler.accept(ErrorHandlers.java:339)
	at org.apache.iceberg.rest.ErrorHandlers$TableErrorHandler.accept(ErrorHandlers.java:160)
	at org.apache.iceberg.rest.ErrorHandlers$TableErrorHandler.accept(ErrorHandlers.java:142)
	at org.apache.iceberg.rest.RESTCatalogAdapter.execute(RESTCatalogAdapter.java:649)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
	at org.apache.iceberg.rest.RESTCatalogAdapter.execute(RESTCatalogAdapter.java:624)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
	at org.apache.iceberg.rest.RESTCatalogAdapter.execute(RESTCatalogAdapter.java:613)
	at org.apache.iceberg.rest.BaseHTTPClient.get(BaseHTTPClient.java:101)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
	at org.apache.iceberg.rest.BaseHTTPClient.get(BaseHTTPClient.java:100)
	at org.apache.iceberg.rest.RESTSessionCatalog.loadInternal(RESTSessionCatalog.java:453)
	at org.apache.iceberg.rest.RESTSessionCatalog.loadTable(RESTSessionCatalog.java:483)
	at org.apache.iceberg.catalog.BaseSessionCatalog$AsCatalog.loadTable(BaseSessionCatalog.java:105)
	at org.apache.iceberg.rest.RESTCatalog.loadTable(RESTCatalog.java:124)
	at org.apache.iceberg.rest.TestRESTCatalog.testTableSnapshotLoadingWithStatisticsOnHistoricalSnapshot(TestRESTCatalog.java:1187)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

[gaborkaszab]

Thanks for opening this @ebyhr !

After I went through the description and checked how suppressHistoricalSnapshots works, I feel that the issue is not with CatalogHandlers not calling discardChanges(). What I'd like to rather scratch is if we should create any statistics removal events when in a "suppress" mode in TableMetada.rewriteSnapshotsInternal(). I don't think so. Not creating removal events in "suppress" mode (similarly as we don't create RemoveSnapshots events either) would solve the issue, right?

[uros-b]

Fix looks minimal and correct to me, thank you @ebyhr and @anoopj