fix: route length/bit_length/octet_length binary input through codegen dispatcher - #5607
fix: route length/bit_length/octet_length binary input through codegen dispatcher#5607adibmbrk wants to merge 1 commit into
Conversation
…n dispatcher length, bit_length, and octet_length rejected BinaryType input and fell the whole projection back to Spark. Mix in CodegenDispatchFallback so the binary case routes through the JVM codegen dispatcher (Spark's own doGenCode) inside the Comet pipeline instead. Docs updated to Hybrid and the SQL fixtures now assert native parity on binary input. Closes apache#5584 Signed-off-by: adibmbrk <adibmbrk@gmail.com>
| object CometLower extends CometCaseConversionBase[Lower]("lower") | ||
|
|
||
| object CometLength extends CometScalarFunction[Length]("length") { | ||
| object CometLength extends CometScalarFunction[Length]("length") with CodegenDispatchFallback { |
There was a problem hiding this comment.
[P2] Preserve child evaluation order for compound binary inputs
This marker dispatches the entire binary-producing child, so the existing kernel null shortcut can now suppress an earlier ANSI error. An unexecuted source-derived diagnostic is IF(flag, length(substring(X'00', CAST(1L DIV 0L AS INT), n)), 0) over persisted Parquet rows (true, NULL) and (false, NULL), with flag BOOLEAN, nullable n INT, and ANSI/Comet projection/codegen dispatch enabled. In the inspected Spark 3.5/4.0 source, the conditional keeps the failing constant inside a branch. Spark evaluates Substring's position before its later length argument, so the selected branch must raise division by zero even when n is null.
Here CometScalaUDF captures the Length tree with only n bound. Its nodes pass allNullIntolerant and the single-input-ordinal guard in CometBatchKernelCodegen, which writes NULL before evaluating the generated child code. At BASE the unsupported binary Length has no dispatcher marker and the enclosing projection falls back to Spark. The new BitLength and OctetLength markers expose the same issue. Please preserve Spark's evaluation order, or retain fallback for these unsafe compound trees, and add a regression asserting the ANSI error for all three roots. This is a source trace, not an executed reproduction.
There was a problem hiding this comment.
Thanks @sunchao, I ran this down and the trace holds. Reproduced on this branch (Spark 4.1, ANSI on):
CREATE TABLE t (flag BOOLEAN, n INT) USING parquet;
INSERT INTO t VALUES (true, NULL), (false, NULL);
SELECT IF(flag, length(substring(X'00', CAST(1L DIV 0L AS INT), n)), 0) FROM t;Spark raises [DIVIDE_BY_ZERO], Comet returns a row. Same for bit_length and octet_length.
For anyone reading later, the three pieces that have to line up:
ConstantFoldingrefuses to fold1L DIV 0Lbecause it sits under anIfbranch (it tagsFAILED_TO_EVALUATEand leaves the node alone), so the throwing literal survives into the physical plan.TernaryExpression.nullSafeCodeGenemitsSubstring'sposcode before it testslen's null, so Spark evaluates the division even thoughnis NULL.Length,Substring,CastandIntegralDivideare all null-intolerant and the dispatched tree reads exactly one ordinal, socanShortCircuitNullstakes its single-ordinal branch and the kernel writes NULL beforeev.coderuns.
One correction on scope: this isn't introduced here, it's the residual hole in #5219. The single-ordinal branch assumes "there is nothing left for Spark to evaluate ahead of that ordinal's own null check", and that's false whenever the tree carries a literal-only subtree that throws. upper reproduces it on main today, unchanged by this PR:
SELECT IF(flag, upper(substring('abc', CAST(1L DIV 0L AS INT), n)), NULL) FROM t;I confirmed that one on the same build: Spark raises, Comet doesn't.
So I'd rather fix canShortCircuitNulls than special-case the three length serdes, otherwise we paper over three of the ~70 expressions that share the hole. Filed as #5608, with the suggested guard and a regression test covering upper plus all three roots from this PR.
@adibmbrk I don't think this needs to block the PR. Please add a link to #5608 in the PR description so the connection isn't lost.
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed 37f12bed after the reproduction discussion. [P2] Thanks for reproducing the evaluation-order issue and tracking the shared fix in #5608. I agree the shared dispatcher is the right place to address it, with these three newly exposed binary callers covered alongside Upper. I found no additional P1/P2 issues.
Could you share one focused binary-input microbenchmark comparing this dispatcher path with the previous Spark-fallback path? Please use matched Spark/Comet build settings, data and warmup, with representative payload widths and null fractions, and include the actual execution plans and matching results. The existing string-expression benchmark does not exercise this binary route.
Which issue does this PR close?
Closes #5584.
Rationale for this change
length,bit_length, andoctet_lengthrejectedBinaryTypeinput and fell the entire projection back to Spark, even though the operation is trivial (numBytes()) andBinaryTypeis already supported by the codegen dispatcher.What changes are included in this PR?
CodegenDispatchFallbackintoCometLength,CometBitLength, andCometOctetLengthso binary input routes through the JVM codegen dispatcher (Spark's owndoGenCode, run inside the Comet pipeline) instead of falling back to Spark.length,len,char_length,character_length,bit_length,octet_length) from Native to Hybrid.How are these changes tested?
Updated the
length.sql,bit_length.sql, andoctet_length.sqlfixtures: the binary cases now assert native-vs-Spark parity (checkSparkAnswerAndOperator) instead ofexpect_fallback. Added binary coverage tolength.sql.Ran against Spark 4.1:
CometStringExpressionSuite— 33 succeeded, 0 failed.CometSqlFileTestSuite— 467 succeeded, 0 failed.