fix: validate map constructor row lengths and null short-circuiting - #5846
fix: validate map constructor row lengths and null short-circuiting#5846sunchao wants to merge 2 commits into
Conversation
andygrove
left a comment
There was a problem hiding this comment.
The per-row check itself looks right, and I confirmed the premise. build_map_array derives offsets only from the non-null keys length and concatenates values, so mismatched row lengths with matching totals really do shift values across map boundaries.
The error it raises is the part I would change. exec_err! produces a plain DataFusion execution error, so it reaches the user as a generic CometNativeException with no error class and no SQLSTATE, while Spark raises a SparkRuntimeException from QueryExecutionErrors.mapDataKeyArrayLengthDiffersFromValueArrayLengthError(). Comet already has SparkError::MapKeyValueDiffSizes in native/common/src/error.rs, and all three ShimSparkErrorConverter variants already route it to exactly that call, so Err(SparkError::MapKeyValueDiffSizes.into()) gives full parity in one line. That is the same pattern SparkMakeDate uses. Would it make sense to use that instead? The assertion in CometMapExpressionSuite only checks the message contains same length, and both wordings do, so it passes either way. Comparing the exception class and error class across the two engines would actually pin it down.
The bigger thing is that this overlaps heavily with #5854. That one rewrites the same CometMapFromArrays.convert and adds its own SparkMapFromArrays re-exported from native/spark-expr/src/map_funcs/mod.rs, so whichever lands second will not compile until they are reconciled. The behavioral part worries me more than the mechanical part. #5854 drops the CaseWhen guard entirely on the grounds that the native kernel is already null intolerant. That is true of the value it returns, but it does not stop the values expression from being evaluated for every row in the batch, which is exactly the ANSI case this PR fixes. Could you and @peterxcli agree an order and decide which implementation survives? A Spark level case where a NULL keys array and a row length mismatch appear in the same batch would stop whichever lands second from quietly dropping the guarantee. Right now that ordering is only pinned by the direct validate_list_lengths calls in the Rust test.
One last thing. The map_from_arrays entry in docs/source/contributor-guide/expression-audits/map_funcs.md still says CometMapFromArrays wraps the inputs in CaseWhen(IsNotNull(left) AND IsNotNull(right), map(left, right), null), which is the exact shape this replaces. Could you update it to describe the nested guards and why the single AND predicate was not enough?
Which issue does this PR close?
Follow-up to #5045, addressing additional correctness failures in
map_from_arrays.Rationale for this change
map_from_arrayspairs each row's keys with that same row's values. Spark requires both arrays to have equal lengths. Comet currently checks the combined data for a batch, allowing mismatches in individual rows to cancel out:[1][10, 20]{1: 10}[2, 3, 4][30, 40]{2: 20, 3: 30, 4: 40}Both columns contain four elements in total, so construction succeeds even though neither row is valid. The value
20crosses into the second row's map. Spark rejects this input with a length-mismatch error; Comet should do the same.There is also an evaluation-order problem. When the keys array is null, Spark returns a null map immediately. For example, consider a batch containing
k = 0andk = 1:With ANSI mode enabled, Spark returns null for
k = 0without evaluating1 / 0. Comet can evaluate the values expression for that row and fail the entire query, even though no map needs to be constructed.What changes are included in this PR?
The native path now checks key/value lengths within each non-null row before constructing maps. The same rule applies when one operand is a constant array repeated across the batch, so combining constants and columns cannot hide a mismatch.
Map construction also follows Spark's null-handling order: check the keys first, evaluate the values only for rows with non-null keys, and construct maps only where both arrays exist. The existing constructor handles the resulting valid inputs. This fixes the two examples above without expanding the set of supported map types or casts. Evaluating nondeterministic children exactly once remains a separate issue in #5781.
How are these changes tested?
On the original PR revision, the Linux Rust test job passed, including native coverage for row-length mismatches, constant/column combinations, nulls, empty batches, and sliced inputs. All six new Spark regressions passed in the Spark 4.1 expression job, with JVM codegen dispatch enabled and disabled.
That run's only failed test was a shared decimal codegen coverage assertion, now corrected by upstream #5849 and included in this branch. The exact assertion failed locally before that correction and passed afterward. Full Spark 4.1 reactor compilation, Scalastyle, and Spotless passed on the updated branch. See fresh CI for revision
f3ee8d91for the complete test matrix.