Repository navigation
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Variant predicates fell back to Spark, and native unshredded scans validated payloads before predicates could apply Spark’s distinct malformed-input behavior.
- Design approach: Add narrowly scoped Variant input serialization and native
is_variant_null/is_valid_variantevaluators. Preserve unshredded bytes while retaining constructor checks. - Correctness / compatibility analysis: Compared Spark 4.0.4, 4.1.3 and 4.2.0 sources, including predicate replacements, accessors and reconstruction. SQL NULL, Variant null, malformed payloads and version gates match the examined behavior. No introduced P1/P2 issues found within this review.
- Key design decisions: Logical Variant metadata guards exclude ordinary structs. Generic Variant consumers retain fallback. The iterative validator follows Spark’s accessors rather than Arrow’s stricter validation rules.
- Implementation sketch: Changes connect the Spark shim, Variant literal protobuf, native planner and scalar functions. Scan options carry Spark’s runtime size limit, with corresponding error conversion and regression tests.
- Behavioral changes worth calling out: Compared with
branch-1.1, supported predicates now execute natively and unshredded reads preserve payloads for consumer validation. These are intended changes. The scan path avoids full payload reconstruction. No throughput benchmark was run, so performance gains are unverified. - Suggested improvements: None meeting the requested P1/P2 evidence threshold.
Reviewed all 34 changed files in the full diff from a4e72fd9d4e3bb8f459fc7950a98536724daf859 to 30508377f258e4df9b4f23773b75b1e0188d0f56. The PR remains non-draft. Routed skills: review-comet-pr, review-comet-expression-pr, and review-comet-ffi-pr. Snapshot and live discussion checks found no existing reviews, comments or unresolved concerns.
Validation: Three native predicate tests passed. Eighteen scan-normalization tests passed in an isolated harness using unchanged source, with one benchmark ignored. A standalone comparison of 34,666 bounded inputs against Spark 4.2’s validator found no differences.
Exact-head CI: Five checks succeeded, two Linux lint checks remain queued, and 14 checks were skipped, including every Spark SQL profile. No completed check reported failure. Local core planner testing was blocked by missing jni.h; JVM integration, Spark SQL suites and throughput benchmarks were not run locally. Those integration results remain unverified.
Which issue does this PR close?
Closes #5429.
Rationale for this change
Evaluate
is_variant_null(Spark 4.0+) andis_valid_variant(Spark 4.2+) natively over whole-value Variant input.What changes are included in this PR?
Add native predicates with Spark's distinct SQL NULL, Variant null and malformed-value behavior. Unshredded scans now preserve payload bytes for the predicates while retaining Spark's required-child, metadata-version and size checks. Shredded reconstruction keeps its existing validation.
How are these changes tested?
All 30 selected native tests and 22 Spark 4.2 tests passed. Coverage includes both dictionary encodings, malformed scan bytes, constructor errors, null masking, canonical/shredded scans and fallback. The previously ignored malformed-scan regression is now active. Updated to current upstream main.