Skip to content

Conversation

@amogh-jahagirdar
Copy link
Collaborator

No description provided.

@github-actions github-actions bot added the CORE label Oct 22, 2024
@rahil-c
Copy link
Owner

rahil-c commented Oct 22, 2024

@amogh-jahagirdar I think some tests are failing otherwise change looks fine to me. Seems to be failing at the parser tests because i think we now are using the fileScanTask.spec() which wont work as is i believe because at the time its unbound file scan tasks in the test. I think either we have to make sure they are bound so this error does not surface.

TestFetchPlanningResultResponseParser > roundTripSerdeWithValidStatusAndFileScanTasks() FAILED
    java.lang.UnsupportedOperationException: spec() is not supported in UnboundBaseFileScanTask
        at org.apache.iceberg.UnboundBaseFileScanTask.spec(UnboundBaseFileScanTask.java:44)
        at org.apache.iceberg.rest.responses.TableScanResponseParser.serializeScanTasks(TableScanResponseParser.java:109)
        at org.apache.iceberg.rest.responses.FetchPlanningResultResponseParser.toJson(FetchPlanningResultResponseParser.java:55)
        at org.apache.iceberg.rest.responses.FetchPlanningResultResponseParser.lambda$toJson$0(FetchPlanningResultResponseParser.java:42)
        at org.apache.iceberg.util.JsonUtil.generate(JsonUtil.java:75)
        at org.apache.iceberg.rest.responses.FetchPlanningResultResponseParser.toJson(FetchPlanningResultResponseParser.java:42)
        at org.apache.iceberg.rest.responses.TestFetchPlanningResultResponseParser.roundTripSerdeWithValidStatusAndFileScanTasks(TestFetchPlanningResultResponseParser.java:203)

@amogh-jahagirdar amogh-jahagirdar force-pushed the scan-plan-model-parser-only-cleanup branch 2 times, most recently from 59ccfc8 to 2f69389 Compare October 31, 2024 04:21

PartitionSpec partitionSpec = specsById.get(fileScanTask.file().specId());
RESTFileScanTaskParser.toJson(fileScanTask, deleteFileReferences, partitionSpec, gen);
PartitionSpec spec = specsById.get(fileScanTask.file().specId());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there also be validation on the map itself specsById to ensure this is not null otherwise this will throw npe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it's good to add validation for that, but not here imo, I think I'd add it a higher level for each response so that it's clear in the failure that a specific response requires the specs by ID to be set when serializing

@amogh-jahagirdar amogh-jahagirdar force-pushed the scan-plan-model-parser-only-cleanup branch from 2f69389 to 7544d4c Compare October 31, 2024 18:51
@rahil-c rahil-c merged commit c68661f into rahil-c:scan-plan-model-parser-only Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants