superdiaodiao commented on PR #17464: URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5584258633
Reworked the tests in 13aa5af — you were right on both counts, and checking them turned up something worth reporting. **"Does this add value? Is there any change in the actual commit results?"** No, there isn't. I removed the guard and re-ran on both sides to check: ``` TestHiveCommits.java:649 -> NPE at BaseMetastoreTableOperations.checkCurrentMetadataLocation TestHiveViewCommits.java:282 -> NPE at HiveViewOperations.checkCurrentMetadataLocation ``` Both failures land on the reflective call, not on the `CommitStateUnknownException` / `tableExists` assertions above it. `checkCommitStatus` swallows the NPE and still reports UNKNOWN, so the observable commit result is identical with and without the fix. The reflection was the only thing seeing a difference — which is exactly your point, so it is gone. **`@VisibleForTesting` package-private:** works for the view (`HiveViewOperations` and `TestHiveViewCommits` share `org.apache.iceberg.hive`), so that side now calls `ops.checkCurrentMetadataLocation(...)` directly. It does **not** work for the table side: `checkCurrentMetadataLocation` lives in `org.apache.iceberg.BaseMetastoreTableOperations` while the test is in `org.apache.iceberg.hive`, so package-private is not visible across the boundary. Rather than widen it further, I took your other suggestion: the null-metadata behaviour is covered by `TestBaseMetastoreTableOperations` in the right package, and `TestHiveCommits` now only pins the Hive-specific piece it is actually there for — that `doRefresh` on a never-persisted table yields null metadata rather than throwing. `checkCurrentMetadataLocation` stays private in core. Net: −33 lines, no reflection. ``` TestBaseMetastoreTableOperations tests=2 failures=0 TestHiveCommits tests=17 failures=0 TestHiveViewCommits tests=13 failures=0 ``` > So other catalogs don't check ATM Correct — no commit-status check at all in `NessieViewOperations`, `JdbcViewOperations` or `RESTViewOperations`, and none in `BaseViewOperations`. Hive is the only view implementation with the retry-on-UNKNOWN path, so it is the only one that can reach this NPE. If another catalog adds that check later it will need the same guard. Also folded the metadata construction into a single `METADATA` constant as suggested. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
