From 37f12bed6ec897b18d344d7d8c409663f642d0b0 Mon Sep 17 00:00:00 2001 From: adibmbrk Date: Tue, 1 Sep 2026 07:21:22 +0530 Subject: [PATCH] fix: route length/bit_length/octet_length binary input through codegen 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 #5584 Signed-off-by: adibmbrk --- docs/source/user-guide/latest/expressions.md | 12 ++++++------ .../scala/org/apache/comet/serde/strings.scala | 17 ++++++++++++++--- .../sql-tests/expressions/string/bit_length.sql | 11 +++++++---- .../sql-tests/expressions/string/length.sql | 17 +++++++++++++++++ .../expressions/string/octet_length.sql | 11 +++++++---- 5 files changed, 51 insertions(+), 17 deletions(-) diff --git a/docs/source/user-guide/latest/expressions.md b/docs/source/user-guide/latest/expressions.md index 2ba71e93c4d..d7579bea45b 100644 --- a/docs/source/user-guide/latest/expressions.md +++ b/docs/source/user-guide/latest/expressions.md @@ -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+) | @@ -574,8 +574,8 @@ 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 | | @@ -583,7 +583,7 @@ The type-name conversion functions (`bigint`, `binary`, `boolean`, `date`, `deci | `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 | | diff --git a/spark/src/main/scala/org/apache/comet/serde/strings.scala b/spark/src/main/scala/org/apache/comet/serde/strings.scala index 665c0d3b54e..0051ac80f40 100644 --- a/spark/src/main/scala/org/apache/comet/serde/strings.scala +++ b/spark/src/main/scala/org/apache/comet/serde/strings.scala @@ -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 { + // 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 { @@ -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 { @@ -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 { diff --git a/spark/src/test/resources/sql-tests/expressions/string/bit_length.sql b/spark/src/test/resources/sql-tests/expressions/string/bit_length.sql index a2327120163..b9fd2abcd23 100644 --- a/spark/src/test/resources/sql-tests/expressions/string/bit_length.sql +++ b/spark/src/test/resources/sql-tests/expressions/string/bit_length.sql @@ -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 @@ -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)) diff --git a/spark/src/test/resources/sql-tests/expressions/string/length.sql b/spark/src/test/resources/sql-tests/expressions/string/length.sql index 1e1e8ccc7a5..fc7807dc74f 100644 --- a/spark/src/test/resources/sql-tests/expressions/string/length.sql +++ b/spark/src/test/resources/sql-tests/expressions/string/length.sql @@ -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 @@ -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)) diff --git a/spark/src/test/resources/sql-tests/expressions/string/octet_length.sql b/spark/src/test/resources/sql-tests/expressions/string/octet_length.sql index e650950347c..5add3b10020 100644 --- a/spark/src/test/resources/sql-tests/expressions/string/octet_length.sql +++ b/spark/src/test/resources/sql-tests/expressions/string/octet_length.sql @@ -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 @@ -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))