Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adibmbrk wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions docs/source/user-guide/latest/expressions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,11 +553,11 @@ The type-name conversion functions (`bigint`, `binary`, `boolean`, `date`, `deci
| --- | --- | --- | --- |
| `ascii` | ✅ | Native | |
| `base64` | ✅ | Native | |
| `bit_length` | ✅ | Native | |
| `bit_length` | ✅ | Hybrid | |
| `btrim` | ✅ | — | |
| `char` | ✅ | Native | |
| `char_length` | ✅ | Native | |
| `character_length` | ✅ | Native | |
| `char_length` | ✅ | Hybrid | |
| `character_length` | ✅ | Hybrid | |
| `chr` | ✅ | Native | |
| `collate` | 🔜 | — | Spark collation (umbrella [#2190](https://github.com/apache/datafusion-comet/issues/2190)) |
| `collation` | ✅ | — | Constant-folded to a literal (Spark 4.0+) |
Expand All @@ -574,16 +574,16 @@ The type-name conversion functions (`bigint`, `binary`, `boolean`, `date`, `deci
| `instr` | ✅ | Native | |
| `lcase` | ✅ | Hybrid | |
| `left` | ✅ | Native | |
| `len` | ✅ | Native | |
| `length` | ✅ | Native | |
| `len` | ✅ | Hybrid | |
| `length` | ✅ | Hybrid | |
| `levenshtein` | ✅ | Native | |
| `locate` | ✅ | Codegen dispatch | |
| `lower` | ✅ | Hybrid | |
| `lpad` | ✅ | — | |
| `ltrim` | ✅ | Native | |
| `luhn_check` | ✅ | — | Native via `StaticInvoke` (tests: luhn_check.sql) |
| `mask` | ✅ | — | Routed through the JVM codegen dispatcher |
| `octet_length` | ✅ | Native | |
| `octet_length` | ✅ | Hybrid | |
| `overlay` | ✅ | Codegen dispatch | |
| `position` | ✅ | Codegen dispatch | |
| `printf` | ✅ | Codegen dispatch | |
Expand Down
17 changes: 14 additions & 3 deletions spark/src/main/scala/org/apache/comet/serde/strings.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,10 @@ object CometUpper extends CometCaseConversionBase[Upper]("upper")

object CometLower extends CometCaseConversionBase[Lower]("lower")

object CometLength extends CometScalarFunction[Length]("length") {
object CometLength extends CometScalarFunction[Length]("length") with CodegenDispatchFallback {

@sunchao sunchao Sep 1, 2026

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.

@andygrove andygrove Sep 1, 2026

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.

// The native `length` UDF has no path for BinaryType. Rather than fall the projection back to
// Spark, route the binary case through the JVM codegen dispatcher (Spark's own `doGenCode`, i.e.
// `numBytes()`) inside the Comet pipeline so the result stays native and matches Spark exactly.
override def getUnsupportedReasons(): Seq[String] = Seq("`BinaryType` input is not supported")

override def getSupportLevel(expr: Length): SupportLevel = expr.child.dataType match {
Expand All @@ -91,7 +94,11 @@ object CometLength extends CometScalarFunction[Length]("length") {
}
}

object CometBitLength extends CometScalarFunction[BitLength]("bit_length") {
object CometBitLength
extends CometScalarFunction[BitLength]("bit_length")
with CodegenDispatchFallback {
// See CometLength: BinaryType has no native path, so route it through the codegen dispatcher
// (Spark's own `doGenCode`, i.e. `numBytes() * 8`) instead of falling back to Spark.
override def getUnsupportedReasons(): Seq[String] = Seq("`BinaryType` input is not supported")

override def getSupportLevel(expr: BitLength): SupportLevel = expr.child.dataType match {
Expand All @@ -100,7 +107,11 @@ object CometBitLength extends CometScalarFunction[BitLength]("bit_length") {
}
}

object CometOctetLength extends CometScalarFunction[OctetLength]("octet_length") {
object CometOctetLength
extends CometScalarFunction[OctetLength]("octet_length")
with CodegenDispatchFallback {
// See CometLength: BinaryType has no native path, so route it through the codegen dispatcher
// (Spark's own `doGenCode`, i.e. `numBytes()`) instead of falling back to Spark.
override def getUnsupportedReasons(): Seq[String] = Seq("`BinaryType` input is not supported")

override def getSupportLevel(expr: OctetLength): SupportLevel = expr.child.dataType match {
Expand Down
11 changes: 7 additions & 4 deletions spark/src/test/resources/sql-tests/expressions/string/bit_length.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
-- specific language governing permissions and limitations
-- under the License.

-- BinaryType has no native path, so it routes through the codegen dispatcher (Spark's own
-- `doGenCode`, i.e. `numBytes() * 8`) instead of falling back to Spark.
-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true

statement
CREATE TABLE test_bit_length(s string) USING parquet

Expand All @@ -28,16 +32,15 @@ SELECT bit_length(s) FROM test_bit_length
query
SELECT bit_length('hello'), bit_length(''), bit_length(NULL)

-- BinaryType input falls back to Spark; the native DataFusion impl rejects Binary at runtime,
-- so the serde gates Binary as Unsupported (matching the existing CometLength shape).
-- BinaryType input routes through the codegen dispatcher and stays inside Comet
statement
CREATE TABLE test_bit_length_binary(b binary) USING parquet

statement
INSERT INTO test_bit_length_binary VALUES (X'48656c6c6f'), (X''), (NULL), (X'FF')

query expect_fallback(bit_length on BinaryType is not supported)
query
SELECT bit_length(b) FROM test_bit_length_binary

query expect_fallback(bit_length on BinaryType is not supported)
query
SELECT bit_length(X'48656c6c6f'), bit_length(CAST(NULL AS BINARY))
17 changes: 17 additions & 0 deletions spark/src/test/resources/sql-tests/expressions/string/length.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
-- specific language governing permissions and limitations
-- under the License.

-- BinaryType has no native path, so it routes through the codegen dispatcher (Spark's own
-- `doGenCode`, i.e. `numBytes()`) instead of falling back to Spark.
-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true

statement
CREATE TABLE test_length(s string) USING parquet

Expand All @@ -27,3 +31,16 @@ SELECT length(s), char_length(s) FROM test_length
-- literal arguments
query
SELECT length('hello'), length(''), length(NULL)

-- BinaryType input routes through the codegen dispatcher and stays inside Comet
statement
CREATE TABLE test_length_binary(b binary) USING parquet

statement
INSERT INTO test_length_binary VALUES (X'48656c6c6f'), (X''), (NULL), (X'FF')

query
SELECT length(b) FROM test_length_binary

query
SELECT length(X'48656c6c6f'), length(CAST(NULL AS BINARY))
11 changes: 7 additions & 4 deletions spark/src/test/resources/sql-tests/expressions/string/octet_length.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
-- specific language governing permissions and limitations
-- under the License.

-- BinaryType has no native path, so it routes through the codegen dispatcher (Spark's own
-- `doGenCode`, i.e. `numBytes()`) instead of falling back to Spark.
-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true

statement
CREATE TABLE test_octet_length(s string) USING parquet

Expand All @@ -28,16 +32,15 @@ SELECT octet_length(s) FROM test_octet_length
query
SELECT octet_length('hello'), octet_length(''), octet_length(NULL)

-- BinaryType input falls back to Spark; the native DataFusion impl rejects Binary at runtime,
-- so the serde gates Binary as Unsupported (matching the existing CometLength shape).
-- BinaryType input routes through the codegen dispatcher and stays inside Comet
statement
CREATE TABLE test_octet_length_binary(b binary) USING parquet

statement
INSERT INTO test_octet_length_binary VALUES (X'48656c6c6f'), (X''), (NULL), (X'FF')

query expect_fallback(octet_length on BinaryType is not supported)
query
SELECT octet_length(b) FROM test_octet_length_binary

query expect_fallback(octet_length on BinaryType is not supported)
query
SELECT octet_length(X'48656c6c6f'), octet_length(CAST(NULL AS BINARY))
Loading