Skip to content

refactor(scan): validate FileScanTask construction - #3131

Merged
CTTY merged 6 commits into
apache:mainfrom
blackmwk:ir-3130
Sep 2, 2026
Merged

refactor(scan): validate FileScanTask construction#3131
CTTY merged 6 commits into
apache:mainfrom
blackmwk:ir-3130

Conversation

@blackmwk

@blackmwk blackmwk commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

What changes are included in this PR?

  • Make FileScanTask fields private and expose read-only public getters.
  • Make FileScanTaskBuilder::build() validate its result without depending on generated typestate arguments.
  • Validate symmetric partition/spec presence and partition tuple arity.
  • Validate partition specs against the task schema by calling PartitionSpec::partition_type directly.
  • Keep task construction lightweight by avoiding per-value partition validation.
  • Update internal call sites for the fallible builder and regenerate the public API snapshot.

Are these changes tested?

  • Added builder regression coverage for non-empty partitions without specs, partitioned specs without values, valid unpartitioned forms, arity mismatches, incompatible evolved schemas, and missing partition source columns.
  • Updated scan and metadata-only Arrow fixtures for the validated construction rules.
  • Ported the construction checks from feat(scan): make FileScanTask serializable #3091 while following review direction to omit per-value validation from this path.
  • cargo test -p iceberg --lib (1,623 passed)
  • cargo clippy --workspace --all-targets -- -D warnings
  • make check-public-api
  • cargo fmt --all -- --check

AI Disclosure

Codex (GPT-5) was used for implementation, call-site refactoring, review follow-up, and unit-test scaffolding. The resulting diff was reviewed against issue #3130, the relevant checks in PR #3091, the latest inline reviews, and repository conventions. All listed checks were run locally; there are no known uncertainties.

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)
Use typed-builder output conversion for validation instead of coupling the custom build method to generated typestate arguments.

Generated-by: Codex (GPT-5)
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)
Comment thread crates/iceberg/src/scan/task.rs Outdated
Comment thread crates/iceberg/src/scan/task.rs Outdated
Comment thread crates/iceberg/src/scan/task.rs Outdated
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)
Comment thread crates/iceberg/src/scan/task.rs Outdated
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)
Comment thread crates/iceberg/src/scan/task.rs Outdated
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)

@CTTY CTTY left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@CTTY
CTTY merged commit 12575c7 into apache:main Sep 2, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the public field accessor of FileScanTask

2 participants