amogh-jahagirdar commented on code in PR #14838:
URL: https://github.com/apache/iceberg/pull/14838#discussion_r2616652391
##########
core/src/main/java/org/apache/iceberg/rest/responses/BaseScanTaskResponse.java:
##########
@@ -23,13 +23,16 @@
import org.apache.iceberg.DeleteFile;
import org.apache.iceberg.FileScanTask;
import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.rest.RESTResponse;
+import org.apache.iceberg.util.DeleteFileSet;
public abstract class BaseScanTaskResponse implements RESTResponse {
private final List<String> planTasks;
private final List<FileScanTask> fileScanTasks;
- private final List<DeleteFile> deleteFiles;
+ private final DeleteFileSet deleteFiles;
Review Comment:
I also think we should be able to get rid of this local state in the
response as well once the deprecated API is removed. The only reason it exists,
is just for the getter to pass back into implementations of
BaseScanTaskResponse. We should also deprecate the public getter for
deleteFiles imo.
##########
core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java:
##########
@@ -77,7 +77,17 @@ public static List<FileScanTask> parseFileScanTasks(
fileScanTaskList.add(fileScanTask);
}
+ if (fileScanTaskList.isEmpty()) {
+ Preconditions.checkArgument(
+ (deleteFiles == null || deleteFiles.isEmpty()),
+ "Invalid response: deleteFiles should only be returned with
fileScanTasks that reference them");
+ }
+
return fileScanTaskList;
+ } else {
+ Preconditions.checkArgument(
+ (deleteFiles == null || deleteFiles.isEmpty()),
+ "Invalid response: deleteFiles should only be returned with
fileScanTasks that reference them");
Review Comment:
Once we properly remove the API in 1.12, we can remove this check from all
the places it's done in validate().
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java:
##########
@@ -196,17 +196,6 @@ public void
roundTripSerdeWithInvalidPlanIdWithIncorrectStatus() {
@Test
public void
roundTripSerdeWithInvalidPlanStatusSubmittedWithDeleteFilesNoFileScanTasksPresent()
{
- assertThatThrownBy(
- () ->
- PlanTableScanResponse.builder()
- .withPlanStatus(PlanStatus.SUBMITTED)
- .withPlanId("somePlanId")
- .withDeleteFiles(List.of(FILE_A_DELETES))
- .build())
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage(
- "Invalid response: deleteFiles should only be returned with
fileScanTasks that reference them");
Review Comment:
I'll undo this, so long as the API exists, we should have this test since a
user could be using it. Once it's removed then we can just remove this too.
##########
core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java:
##########
@@ -77,7 +77,17 @@ public static List<FileScanTask> parseFileScanTasks(
fileScanTaskList.add(fileScanTask);
}
+ if (fileScanTaskList.isEmpty()) {
+ Preconditions.checkArgument(
+ (deleteFiles == null || deleteFiles.isEmpty()),
+ "Invalid response: deleteFiles should only be returned with
fileScanTasks that reference them");
+ }
+
return fileScanTaskList;
+ } else {
+ Preconditions.checkArgument(
+ (deleteFiles == null || deleteFiles.isEmpty()),
+ "Invalid response: deleteFiles should only be returned with
fileScanTasks that reference them");
Review Comment:
I think this is the real place to do this check. It makes sense to validate
on parsing JSON coming over the wire. It does not make sense to do this check
after building the response because our builder shouldn't even allow that
situation to happen.
--
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]
