liamzwbao commented on code in PR #1126:
URL: https://github.com/apache/polaris/pull/1126#discussion_r2003488989
##########
service/common/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java:
##########
@@ -87,10 +87,7 @@ private boolean cleanUpManifestFile(
StreamSupport.stream(
Spliterators.spliteratorUnknownSize(dataFiles.iterator(),
Spliterator.IMMUTABLE),
false)
- .map(
- file ->
- tryDelete(
- tableId, fileIO, manifestFile.path(),
file.path().toString(), null, 1))
+ .map(file -> tryDelete(tableId, fileIO, manifestFile.path(),
file.location(), null, 1))
Review Comment:
`file.path()` is deprecated in 1.8 and replaced by `file.location()`. See
[link](https://github.com/apache/iceberg/blob/82044624da9d4e45c94b962659acd3db5378a8fa/api/src/main/java/org/apache/iceberg/ContentFile.java#L54-L65)
##########
integration-tests/src/main/java/org/apache/polaris/service/it/env/IcebergHelper.java:
##########
@@ -53,11 +54,11 @@ public static RESTCatalog restCatalog(
.put(
org.apache.iceberg.CatalogProperties.FILE_IO_IMPL,
"org.apache.iceberg.inmemory.InMemoryFileIO")
- .put("warehouse", catalog)
+ .put("warehouse", warehouse)
Review Comment:
I'm not very familiar with the relationship between the warehouse and the
catalog here, but based on my observations, some tests use the same client to
create multiple catalogs while keeping the warehouse unchanged, so I think
their names can be different.
If the warehouse name is set to match the catalog name for this
`IcebergHelper`, it may lead to a "warehouse not found" error. For example, in
the test
[testCatalogWithCustomMetricsReporter](https://github.com/apache/iceberg/blob/58b283e3533afce8b9884ac847a46eb40688353b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java#L2936),
initCatalog is called with a catalog name `catalog_with_custom_reporter` that
is different from default. This causes the test to search for a warehouse named
`catalog_with_custom_reporter`, which doesn’t exist, as the warehouse was
already initialized with a different name in `@BeforeEach` and didn't change.
I'm not sure whether we could set warehouse name to a constant and we only
change the catalog name in the tests
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/task/TaskTestUtils.java:
##########
@@ -110,7 +109,7 @@ static TableMetadata writeTableMetadata(
for (Snapshot snapshot : snapshots) {
tmBuilder.addSnapshot(snapshot);
if (statisticsFiles != null) {
- tmBuilder.setStatistics(snapshot.snapshotId(),
statisticsFiles.get(statisticsFileIndex++));
+ tmBuilder.setStatistics(statisticsFiles.get(statisticsFileIndex++));
Review Comment:
`setStatistics(long snapshotId, StatisticsFile statisticsFile` is deprecated
and replaced by `setStatistics(StatisticsFile statisticsFile)`. See
[link](https://github.com/apache/iceberg/blob/82044624da9d4e45c94b962659acd3db5378a8fa/api/src/main/java/org/apache/iceberg/UpdateStatistics.java#L32-L45)
--
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]
