Skip to content

fix: route length/bit_length/octet_length binary input through codegen dispatcher - #5607

Open
adibmbrk wants to merge 1 commit into
apache:mainfrom
adibmbrk:binary-length-codegen-dispatch
Open

fix: route length/bit_length/octet_length binary input through codegen dispatcher#5607
adibmbrk wants to merge 1 commit into
apache:mainfrom
adibmbrk:binary-length-codegen-dispatch

Conversation

@adibmbrk

@adibmbrk adibmbrk commented Sep 1, 2026

Copy link
Copy Markdown

Which issue does this PR close?

Closes #5584.

Rationale for this change

length, bit_length, and octet_length rejected BinaryType input and fell the entire projection back to Spark, even though the operation is trivial (numBytes()) and BinaryType is already supported by the codegen dispatcher.

What changes are included in this PR?

  • Mix CodegenDispatchFallback into CometLength, CometBitLength, and CometOctetLength so binary input routes through the JVM codegen dispatcher (Spark's own doGenCode, run inside the Comet pipeline) instead of falling back to Spark.
  • Regenerate the affected docs rows (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, and octet_length.sql fixtures: the binary cases now assert native-vs-Spark parity (checkSparkAnswerAndOperator) instead of expect_fallback. Added binary coverage to length.sql.

Ran against Spark 4.1:

  • CometStringExpressionSuite — 33 succeeded, 0 failed.
  • CometSqlFileTestSuite — 467 succeeded, 0 failed.

…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 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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:

  • ConstantFolding refuses to fold 1L DIV 0L because it sits under an If branch (it tags FAILED_TO_EVALUATE and leaves the node alone), so the throwing literal survives into the physical plan.
  • TernaryExpression.nullSafeCodeGen emits Substring's pos code before it tests len's null, so Spark evaluates the division even though n is NULL.
  • Length, Substring, Cast and IntegralDivide are all null-intolerant and the dispatched tree reads exactly one ordinal, so canShortCircuitNulls takes its single-ordinal branch and the kernel writes NULL before ev.code runs.

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 sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

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.

length / bit_length / octet_length fall back to Spark on binary input

3 participants