You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by ag...@apache.org on 2026/06/03 22:22:21 UTC
(datafusion-comet) branch main updated: fix: codegen dispatcher returns NULL for invalid try_make_timestamp inputs (#4554) (#4579)
This is an automated email from the ASF dual-hosted git repository.
andygrove pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion-comet.git
The following commit(s) were added to refs/heads/main by this push:
new 9e31a6123a fix: codegen dispatcher returns NULL for invalid try_make_timestamp inputs (#4554) (#4579)
9e31a6123a is described below
commit 9e31a6123a9caa5d9d13945f644ded3f148215c4
Author: Andy Grove <ag...@apache.org>
AuthorDate: Wed Jun 3 16:22:17 2026 -0600
fix: codegen dispatcher returns NULL for invalid try_make_timestamp inputs (#4554) (#4579)
---
docs/source/user-guide/latest/expressions.md | 2 +-
.../comet/codegen/CometBatchKernelCodegen.scala | 40 +++++++++++----
.../expressions/datetime/make_timestamp.sql | 31 ++++++++++++
.../expressions/datetime/try_make_timestamp.sql | 58 ++++++++++++++++++++++
.../org/apache/comet/CometCodegenSourceSuite.scala | 40 +++++++++++++++
5 files changed, 161 insertions(+), 10 deletions(-)
diff --git a/docs/source/user-guide/latest/expressions.md b/docs/source/user-guide/latest/expressions.md
index 916d4a1785..b5ffd40161 100644
--- a/docs/source/user-guide/latest/expressions.md
+++ b/docs/source/user-guide/latest/expressions.md
@@ -283,7 +283,7 @@ The type-name conversion functions (`bigint`, `binary`, `boolean`, `date`, `deci
| `to_utc_timestamp` | ✅ | Legacy zone forms fall back (Incompatible) ([details](compatibility/expressions/datetime.md)) |
| `trunc` | ✅ | |
| `try_make_interval` | 🔜 | Produces legacy CalendarInterval; tracked by [#4540](https://github.com/apache/datafusion-comet/issues/4540) |
-| `try_make_timestamp` | ⚠️ | Returns a wrong value instead of NULL for invalid inputs ([#4554](https://github.com/apache/datafusion-comet/issues/4554)) |
+| `try_make_timestamp` | ✅ | |
| `try_to_date` | 🔜 | Rewrites to `Cast`/`GetTimestamp` but currently falls back; tracked by [#4556](https://github.com/apache/datafusion-comet/issues/4556) |
| `try_to_time` | 🔜 | Spark 4.1 TIME type; tracked by [#4288](https://github.com/apache/datafusion-comet/issues/4288) |
| `try_to_timestamp` | 🔜 | Rewrites to `Cast`/`GetTimestamp` but currently falls back; tracked by [#4556](https://github.com/apache/datafusion-comet/issues/4556) |
diff --git a/spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegen.scala b/spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegen.scala
index 23d170743e..25bd70898e 100644
--- a/spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegen.scala
+++ b/spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegen.scala
@@ -365,15 +365,37 @@ object CometBatchKernelCodegen extends Logging with CometExprTraitShim {
val nullCheck =
if (inputOrdinals.isEmpty) "false"
else inputOrdinals.map(nullCheckCall).mkString(" || ")
- s"""
- |if ($nullCheck) {
- | output.setNull(i);
- |} else {
- | $subExprsCode
- | ${ev.code}
- | $writeSnippet
- |}
- """.stripMargin
+ // `NullIntolerant` only constrains "any input null -> output null"; it does NOT promise
+ // that non-null inputs always produce non-null output. `MakeTimestamp(failOnError=false)`
+ // is `NullIntolerant=true` but its `doGenCode` catches `DateTimeException` for invalid
+ // year/month/day/hour/min/sec components and sets `ev.isNull = true`. Honor `ev.isNull`
+ // post-eval whenever the expression is nullable; skip the guard only when the root is
+ // statically non-nullable (`ev.isNull` is then a literal `false`).
+ if (boundExpr.nullable) {
+ s"""
+ |if ($nullCheck) {
+ | output.setNull(i);
+ |} else {
+ | $subExprsCode
+ | ${ev.code}
+ | if (${ev.isNull}) {
+ | output.setNull(i);
+ | } else {
+ | $writeSnippet
+ | }
+ |}
+ """.stripMargin
+ } else {
+ s"""
+ |if ($nullCheck) {
+ | output.setNull(i);
+ |} else {
+ | $subExprsCode
+ | ${ev.code}
+ | $writeSnippet
+ |}
+ """.stripMargin
+ }
case _ =>
// NonNullableOutputShortCircuit: when `nullable = false`, drop the `if (ev.isNull)`
// guard at source level rather than relying on JIT folding.
diff --git a/spark/src/test/resources/sql-tests/expressions/datetime/make_timestamp.sql b/spark/src/test/resources/sql-tests/expressions/datetime/make_timestamp.sql
index 07f4ffd8c8..11fee8d582 100644
--- a/spark/src/test/resources/sql-tests/expressions/datetime/make_timestamp.sql
+++ b/spark/src/test/resources/sql-tests/expressions/datetime/make_timestamp.sql
@@ -40,3 +40,34 @@ SELECT make_timestamp(y, mo, d, h, mi, s, 'UTC') FROM test_make_timestamp
-- literal arguments
query
SELECT make_timestamp(2024, 6, 15, 10, 30, 45.000)
+
+-- Invalid components: with the default failOnError=false (ANSI off), out-of-range values must
+-- return NULL, not garbage. The codegen dispatcher previously skipped MakeTimestamp's post-eval
+-- `ev.isNull` guard for this null-intolerant expression and wrote stale ev.value bytes; this
+-- regressed try_make_timestamp (which rewrites to MakeTimestamp(failOnError=false)) and the
+-- non-ANSI MakeTimestamp path. See issue #4554.
+statement
+CREATE TABLE test_make_timestamp_invalid(y int, mo int, d int, h int, mi int, s decimal(8,6)) USING parquet
+
+statement
+INSERT INTO test_make_timestamp_invalid VALUES
+ (2024, 13, 1, 0, 0, 0.0),
+ (2024, 2, 30, 0, 0, 0.0),
+ (2024, 6, 32, 0, 0, 0.0),
+ (2024, 6, 15, 25, 0, 0.0),
+ (2024, 6, 15, 0, 60, 0.0),
+ (2024, 6, 15, 10, 30, 45.0)
+
+query
+SELECT make_timestamp(y, mo, d, h, mi, s) FROM test_make_timestamp_invalid
+
+-- Invalid literal components must also return NULL (not throw, not return garbage).
+query
+SELECT make_timestamp(2024, 13, 1, 0, 0, 0.0)
+
+query
+SELECT make_timestamp(2024, 2, 30, 0, 0, 0.0)
+
+query
+SELECT make_timestamp(2024, 6, 15, 25, 0, 0.0)
+
diff --git a/spark/src/test/resources/sql-tests/expressions/datetime/try_make_timestamp.sql b/spark/src/test/resources/sql-tests/expressions/datetime/try_make_timestamp.sql
new file mode 100644
index 0000000000..029f4fa91b
--- /dev/null
+++ b/spark/src/test/resources/sql-tests/expressions/datetime/try_make_timestamp.sql
@@ -0,0 +1,58 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- try_make_timestamp rewrites to MakeTimestamp(failOnError = false), which the codegen
+-- dispatcher routes through Spark's MakeTimestamp.doGenCode. Invalid components must produce
+-- NULL, not garbage timestamp bytes (issue #4554).
+--
+-- Function added in Spark 4.0.0.
+-- MinSparkVersion: 4.0
+-- Config: spark.sql.session.timeZone=UTC
+-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true
+
+statement
+CREATE TABLE test_try_make_ts(y int, mo int, d int, h int, mi int, s decimal(8,6)) USING parquet
+
+statement
+INSERT INTO test_try_make_ts VALUES
+ (2024, 6, 15, 10, 30, 45.123456),
+ (2024, 13, 1, 0, 0, 0.0),
+ (2024, 2, 30, 0, 0, 0.0),
+ (2024, 6, 32, 0, 0, 0.0),
+ (2024, 6, 15, 25, 0, 0.0),
+ (2024, 6, 15, 0, 60, 0.0),
+ (NULL, 6, 15, 10, 30, 45.0)
+
+query
+SELECT try_make_timestamp(y, mo, d, h, mi, s) FROM test_try_make_ts
+
+-- Literal invalid components must also return NULL.
+query
+SELECT try_make_timestamp(2024, 13, 1, 0, 0, 0.0)
+
+query
+SELECT try_make_timestamp(2024, 6, 32, 0, 0, 0.0)
+
+query
+SELECT try_make_timestamp(2024, 6, 15, 25, 0, 0.0)
+
+-- Sentinel: a valid input must run on the Comet codegen path. If the dispatcher silently
+-- rejected MakeTimestamp the operator would fall back to Spark and the NULL-on-invalid queries
+-- above pass vacuously. `query` mode uses checkSparkAnswerAndOperator, which fails if Comet
+-- did not run the expression natively.
+query
+SELECT try_make_timestamp(2024, 6, 15, 10, 30, 45.0)
diff --git a/spark/src/test/scala/org/apache/comet/CometCodegenSourceSuite.scala b/spark/src/test/scala/org/apache/comet/CometCodegenSourceSuite.scala
index a609ccbfe2..b9c5175117 100644
--- a/spark/src/test/scala/org/apache/comet/CometCodegenSourceSuite.scala
+++ b/spark/src/test/scala/org/apache/comet/CometCodegenSourceSuite.scala
@@ -393,6 +393,46 @@ class CometCodegenSourceSuite extends AnyFunSuite {
CodeFormatter.format(result.code))
}
+ test("nullable NullIntolerant root keeps post-eval isNull guard inside short-circuit (#4554)") {
+ // `NullIntolerant` only constrains "null in -> null out". An expression can still set
+ // `ev.isNull = true` from non-null inputs — `MakeTimestamp(failOnError = false)` does this in
+ // its `doGenCode` catch block when year/month/day/hour/min/sec components are out of range
+ // (issue #4554). The dispatcher previously assumed NullIntolerant + non-null inputs implied a
+ // non-null output and dropped the post-eval guard; that wrote stale `ev.value` bytes for
+ // every invalid row. The short-circuit on input nulls must coexist with a post-eval
+ // `if (ev.isNull) setNull` check whenever the expression itself is nullable.
+ val expr = MakeTimestamp(
+ BoundReference(0, IntegerType, nullable = true),
+ BoundReference(1, IntegerType, nullable = true),
+ BoundReference(2, IntegerType, nullable = true),
+ BoundReference(3, IntegerType, nullable = true),
+ BoundReference(4, IntegerType, nullable = true),
+ BoundReference(5, DecimalType(8, 6), nullable = true),
+ timezone = None,
+ timeZoneId = Some("UTC"),
+ failOnError = false)
+ assert(expr.nullable, "MakeTimestamp(failOnError=false) must be nullable for this test")
+ val intCol = ArrowColumnSpec(
+ CometBatchKernelCodegen.vectorClassBySimpleName("IntVector"),
+ nullable = true)
+ val decCol = ArrowColumnSpec(
+ CometBatchKernelCodegen.vectorClassBySimpleName("DecimalVector"),
+ nullable = true)
+ val result = CometBatchKernelCodegen.generateSource(
+ expr,
+ IndexedSeq(intCol, intCol, intCol, intCol, intCol, decCol))
+ val src = result.body
+ val formatted = CodeFormatter.format(result.code)
+ // Two distinct setNull sites must exist: the input-null short-circuit before `ev.code` runs,
+ // and the post-eval guard that propagates `ev.isNull = true` set by MakeTimestamp's catch
+ // block on invalid components. Pre-fix there was only one (the input short-circuit).
+ val setNullOccurrences = "output\\.setNull\\(i\\);".r.findAllIn(src).length
+ assert(
+ setNullOccurrences >= 2,
+ "expected at least two setNull sites (input short-circuit + post-eval ev.isNull guard); " +
+ s"found $setNullOccurrences. Source:\n$formatted")
+ }
+
test("ArrayType(StringType) output emits ListVector startNewValue/endValue recursion") {
// CreateArray over a BoundReference(StringType) produces ArrayType(StringType). emitWrite's
// ArrayType case should emit:
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org