feat!: validate default evaluator input schemas at top level - #3274
feat!: validate default evaluator input schemas at top level#3274dengsh12 wants to merge 7 commits into
Conversation
github-actions
Bot
added
the
breaking-change
dengsh12
changed the title
DefaultExpressionEvaluator
92b9a70 to
0b9a1ab
Compare
github-actions
Bot
removed
the
breaking-change
github-actions
Bot
added
the
breaking-change
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3274 +/- ##
==========================================
+ Coverage 90.26% 90.36% +0.10%
==========================================
Files 251 250 -1
Lines 88309 89231 +922
Branches 88309 89231 +922
==========================================
+ Hits 79710 80632 +922
+ Misses 5727 5660 -67
- Partials 2872 2939 +67 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dengsh12
changed the title
Benchmark results: ✅ PassSummary: 🚀 0 · ✅ 8 · ☑️ 2 · 🚧 5 · ❌ 0 Per-benchmark results (15 rows)
Legend: 🚀 ≥1.15x faster · ✅ faster or unchanged · ☑️ ≤1.03x slower · 🚧 1.03x-1.15x slower · ❌ ≥1.15x slower |
| } | ||
| } | ||
|
|
||
| fn validate_data_schema_top_level( |
There was a problem hiding this comment.
if data_schema has two fields with same names, it won't be rejected here. AFAIK we must never create such arrow schema -- the validation seems unnecessary. But lmk if you think there is a need
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This PR turns on the previously dead top-level input-schema check in the default Arrow evaluator and threads explicit read schemas through the checkpoint, sequential, and remove-action paths. No blocking issues. The type-compatibility mappings (interval, variant, dictionary, list/map variants, string/int width normalization) line up with the kernel Arrow conversion in both directions, sidecar discovery still gets SIDECAR_FIELD, and the four-evaluator remove selection matches the producer field order. A few non-blocking notes follow.
Non-blocking notes
Nit3
kernel/src/engine/arrow_expression/mod.rs:446 (RIGHT). top_level_types_compatible hand-encodes the interval and variant relationships that are the inverse of the Arrow-to-kernel mapping already defined in arrow_conversion. The special cases are needed because conversion erases those logical distinctions, but the table is a second copy of that correspondence and will need updating whenever a new type or alias is added there. Raised by: architecture-reviewer. Suggested fix: consider co-locating the "which Arrow types normalize to kernel type X" knowledge with the conversion helpers so this check consumes it rather than restating it; not required for this change.
Automated review - workflow run
| })?; | ||
| // Only the top-level type is validated. `try_from_arrow` translates the entire field, but | ||
| // we use it here to keep the validation simple. | ||
| let data_field = StructField::try_from_arrow(data_field.as_ref())?; |
There was a problem hiding this comment.
Nit1 The comment says "Only the top-level type is validated," but StructField::try_from_arrow recursively converts the entire matched field. If a matched top-level field contains a nested Arrow type with no kernel equivalent (Duration, native Interval, Timestamp(Second, _), Time32/64, Decimal256), the conversion errors and the batch is rejected, even though only the container kind matters and the same type in an unmatched extra field is tolerated. It also runs a full nested translation per evaluate() call on the log-replay path. Low likelihood today since data originates from kernel reads, but it is a latent false-reject that contradicts the stated intent. Raised by: maintainer-claude-reviewer, delta-protocol-reviewer. Suggested fix: classify on the Arrow DataType discriminant (Struct/List/.../Map/primitive) and convert only the top-level/leaf type, and add a test with a nested non-convertible Arrow type under a matched struct field.
| let remove_actions = self.generate_remove_actions( | ||
| engine, | ||
| self.dv_matched_files.iter(), | ||
| true, /* has_dv_update_columns */ |
There was a problem hiding this comment.
Nit2 The DV-update path calls generate_remove_actions(..., true /* has_dv_update_columns */), but every update_deletion_vectors test builds its scan without stats_parsed/partitionValues_parsed, so only evaluator index 0 is exercised. The (true,false), (false,true), and (true,true) branches of scan_row_input_schema(_, _, true) are constructed but never evaluated against real data. Raised by: test-coverage-reviewer. Suggested fix: parametrize a DV-update integration test with StatsOptions/PartitionValuesOptions (mirroring test_remove_files_partitioned_with_parsed_columns), or add a unit test asserting scan_row_input_schema(true, true, true) yields exactly the scan-row fields plus stats_parsed, partitionValues_parsed, and the two DV columns.
| Ok(()) | ||
| } | ||
|
|
||
| fn top_level_types_compatible(expected_type: &DataType, data_type: &DataType) -> bool { |
There was a problem hiding this comment.
Nit3 top_level_types_compatible hand-encodes the interval and variant relationships that are the inverse of the Arrow-to-kernel mapping already defined in arrow_conversion. The special cases are needed because conversion erases those logical distinctions, but the table is a second copy that will need updating whenever a new type or alias is added there. Raised by: architecture-reviewer. Suggested fix: consider co-locating the "which Arrow types normalize to kernel type X" knowledge with the conversion helpers so this check consumes it rather than restating it; not required for this change.
| .collect::<Vec<_>>() | ||
| )) | ||
| })?; | ||
| // Only the top-level type is validated. `try_from_arrow` translates the entire field, but |
There was a problem hiding this comment.
O(schema) translation, from the PR benchmark seems the perf impact not observable? we will do O(schema) validation anyway in future...
1 participant
What changes are proposed in this pull request?
Short term solution mentioned in #3263
How was this change tested?
positive test: all kernel data types; nested mismatches
negative test: all validation failure cases including missing, mismatching fields