feat(scan): make FileScanTask serializable - #3091
Conversation
Replace the placeholder serde errors with a lossless representation for partition literals and use the existing serde implementations for the remaining scan context fields. Expand round-trip coverage to include a fully populated task and every primitive partition literal. [apache#3089](apache#3089)
Move the self-describing serde representation into Literal so scan task partitions use the shared values-layer implementation. Cover primitive and nested literal round trips and remove the task-local helper requested in review.
Record the new Serialize and Deserialize implementations for Literal and Struct.
Serialize partition data through the existing RawLiteral implementation using the task schema and partition spec. Remove the direct Literal and Struct serde implementation and keep the public API unchanged.
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Nice to see this one finally get a real serde path — the mirror-struct approach is clean, and the move to full-struct assert_eq! round-trips is a real step up from the old field-by-field checks.
I'd hold it before merging, though. The partition encoding is the load-bearing new behavior here, and there's a case that breaks a straight Rust-to-Rust round-trip.
The main concern is timestamp_ns / timestamptz_ns partition columns. Both are backed by PrimitiveLiteral::Long, so they serialize as JSON integers fine, but the Long arm in values/serde.rs has no branch to turn them back into a literal on deserialize — so any V3 table partitioned on a nanosecond timestamp produces FileScanTasks that fail to deserialize once this lands. This is the first caller to actually exercise that path, which is why the gap surfaces here. I'd add the two missing arms in serde.rs and a round-trip test that pins them.
A few things I'd want to settle before merge:
- Add the
TimestampNs/TimestamptzNsarms invalues/serde.rsand atimestamp_nspartition round-trip test. - Pin the
partition = Some/partition_spec = Nonebehavior with an explicit guard + test — right now it either silently drops the partition values or errors opaquely, and nobody's sure which. - Handle (or at least document) partition-evolution + dropped-source-column, which currently makes serialize fail.
- Drop the per-serialize
self.clone()in favor ofTryFrom<&FileScanTask>, addskip_serializing_iftorecord_count, and swapuse super::*for explicit imports to match the other_serdemodules.
One more worth a look but I won't block on it: the Record arm in serde.rs rebuilds the partition struct in JSON field order rather than schema order, so a non-Rust JSON producer could map values to the wrong partition slots. serde_json round-trips fine, so it doesn't bite the current use case, but it's fragile enough to be worth a follow-up.
Once those are addressed, happy to take another pass and approve.
Validate partition arity and transformed value types while preserving support for historical partition specs whose source columns were dropped. Port the partition coverage from PR apache#3091 to builder regression tests. Generated-by: Codex (GPT-5)
|
hi, @laskoviymishka thanks for the careful review. After second thought I think we should not do validation in the ser/de path, instead we should do it when the build happens. We should merge #3131 first |
* refactor(scan): Validate FileScanTask construction Make FileScanTask fields private, expose read-only getters, and validate builder output so non-empty partition data always has an associated partition spec. Generated-by: Codex (GPT-5) * refactor(scan): Simplify FileScanTask validation Use typed-builder output conversion for validation instead of coupling the custom build method to generated typestate arguments. Generated-by: Codex (GPT-5) * fix(scan): Validate FileScanTask partition data Validate partition arity and transformed value types while preserving support for historical partition specs whose source columns were dropped. Port the partition coverage from PR #3091 to builder regression tests. Generated-by: Codex (GPT-5) * fix(scan): Refine partition validation Enforce symmetric partition/spec presence and tuple arity while avoiding per-value transform validation during task construction. Update the spec-id scan fixture to provide its partition tuple. Generated-by: Codex (GPT-5) * fix(scan): Validate partition specs against schema Preserve historical specs with dropped source columns while validating resolvable partition specs against the task schema. Add regression coverage for an incompatible evolved schema. Generated-by: Codex (GPT-5) * fix(scan): Always validate partition specs Remove the dropped-source shortcut and rely directly on PartitionSpec::partition_type for schema validation. Update dropped-source coverage to assert the resulting planning error. Generated-by: Codex (GPT-5)
# Conflicts: # crates/iceberg/src/scan/mod.rs # crates/iceberg/src/scan/task.rs
|
I think this pr is ready for review, cc @laskoviymishka @CTTY @kevinjqliu PTAL |
4 participants
Which issue does this PR close?
What changes are included in this PR?
FileScanTaskcontext fields with working serialization.FileScanTaskthat serializes partition data through the existingRawLiteralimplementation.The existing
key_metadatatrust boundary is unchanged: encrypted-file key metadata remains part of a serialized scan task as documented on the field.Are these changes tested?
cargo test -p iceberg --lib(1,585 passed)cargo clippy -p iceberg --lib --tests -- -D warningscargo fmt --all -- --checkgit diff --checkmake check-public-apiAI Disclosure
Codex was used to assist with implementation, regression-test scaffolding, review follow-up, and validation. I reviewed the resulting design and diff, and verified it with the tests and linters listed above. There are no known unresolved assumptions or uncertainties.