Copilot commented on code in PR #11219:
URL: https://github.com/apache/gravitino/pull/11219#discussion_r3300565197
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -528,6 +490,16 @@ public PlanTableScanResponse planTableScan(
}
}
+ @SuppressWarnings("deprecation")
+ private static PlanTableScanResponse buildCompletedPlanTableScanResponse(
+ List<FileScanTask> fileScanTasks, Map<Integer, PartitionSpec> specsById)
{
+ return PlanTableScanResponse.builder()
+ .withPlanStatus(PlanStatus.COMPLETED)
+ .withFileScanTasks(fileScanTasks)
+ .withSpecsById(specsById)
+ .build();
+ }
Review Comment:
The new builder helper is annotated with `@SuppressWarnings("deprecation")`,
indicating that at least one of `withFileScanTasks` / `withSpecsById` is
already deprecated in Iceberg 1.11. Please add a brief comment (and ideally a
TODO referencing the Iceberg issue/PR) explaining which API is deprecated and
what the migration path is, so this does not silently rot at the next Iceberg
upgrade.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java:
##########
@@ -159,11 +159,13 @@ void testNamespaceExists() {
verifyNamespaceExistsStatusCode(204, Namespace.of("exists_foo2", "a"));
verifyNamespaceExistsStatusCode(404, Namespace.of("exists_foo2", "b"));
Review Comment:
This is a meaningful behavioral change: dropping a non-empty namespace now
returns 409 instead of silently succeeding (as a consequence of the Iceberg
1.11 upgrade). Please add a short comment in this test explaining the new
expectation (e.g., "Dropping a namespace with child namespaces returns 409
since Iceberg 1.11"), so future readers do not assume the test was simply
rewritten for style.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java:
##########
@@ -52,6 +53,9 @@ public class IcebergRESTUtils {
private static final Logger LOG =
LoggerFactory.getLogger(IcebergRESTUtils.class);
+ /** UTF-8 URL-encoded namespace separator used by the Iceberg REST catalog
spec. */
Review Comment:
It would help future maintainers to expand this comment to explain *why* a
non-default separator is being passed to
`RESTUtil.encodeNamespace/decodeNamespace` (i.e., Iceberg 1.11 changed the
default and Gravitino is preserving the URL-encoded `0x1F` separator on the
wire), and to note that all server-side decoding and client-side encoding must
use the same value. Without this context the constant looks like a magic string.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -454,62 +452,26 @@ public PlanTableScanResponse planTableScan(
return cachedResponse.get();
}
- List<String> planTasks = new ArrayList<>();
- Map<Integer, PartitionSpec> specsById = new HashMap<>();
- List<DeleteFile> deleteFiles = new ArrayList<>();
+ List<FileScanTask> fileScanTasksList = new ArrayList<>();
try (CloseableIterable<FileScanTask> fileScanTasks =
createFilePlanScanTasks(table, tableIdentifier, scanRequest)) {
for (FileScanTask fileScanTask : fileScanTasks) {
- try {
- String taskString = ScanTaskParser.toJson(fileScanTask);
- planTasks.add(taskString);
-
- int specId = fileScanTask.spec().specId();
- if (!specsById.containsKey(specId)) {
- specsById.put(specId, fileScanTask.spec());
- }
-
- if (!fileScanTask.deletes().isEmpty()) {
- deleteFiles.addAll(fileScanTask.deletes());
- }
- } catch (Exception e) {
- throw new RuntimeException(
- String.format(
- "Failed to serialize scan task for table: %s. Error: %s",
- tableIdentifier, e.getMessage()),
- e);
- }
+ fileScanTasksList.add(fileScanTask);
}
} catch (IOException e) {
LOG.error("Failed to close scan task iterator for table: {}",
tableIdentifier, e);
throw new RuntimeException("Failed to plan scan tasks: " +
e.getMessage(), e);
}
- List<DeleteFile> uniqueDeleteFiles =
- deleteFiles.stream().distinct().collect(Collectors.toList());
-
- if (planTasks.isEmpty()) {
+ if (fileScanTasksList.isEmpty()) {
LOG.info(
"Scan planning returned no tasks for table: {}. Table may be empty
or fully filtered.",
tableIdentifier);
}
- PlanTableScanResponse.Builder responseBuilder =
- PlanTableScanResponse.builder()
- .withPlanStatus(PlanStatus.COMPLETED)
- .withPlanTasks(planTasks)
- .withSpecsById(specsById);
-
- if (!uniqueDeleteFiles.isEmpty()) {
- responseBuilder.withDeleteFiles(uniqueDeleteFiles);
- LOG.debug(
- "Included {} delete files in scan plan for table: {}",
- uniqueDeleteFiles.size(),
- tableIdentifier);
- }
-
- PlanTableScanResponse response = responseBuilder.build();
+ PlanTableScanResponse response =
+ buildCompletedPlanTableScanResponse(fileScanTasksList,
table.specs());
Review Comment:
The previous implementation populated `specsById` only with the partition
specs actually referenced by the planned `FileScanTask`s. The new code passes
`table.specs()` unconditionally, which is the full historical map of all specs
the table has ever had. For tables that have undergone partition evolution,
this can return many more specs than the response needs and may include specs
that the client does not actually require to interpret the returned tasks.
Consider building the spec map from the scanned tasks (as before) or at least
restricting it to the current spec, to keep the response minimal and consistent
with the prior behavior.
--
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]
