sunchao commented on code in PR #7117: URL: https://github.com/apache/datafusion-comet/pull/7117#discussion_r4317025168
########## spark/src/test/resources/sql-tests/expressions/datetime/make_date_overflow.sql: ########## @@ -0,0 +1,109 @@ +-- 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. + +-- ConfigMatrix: spark.sql.ansi.enabled=false,true +-- Config: spark.comet.exec.scalaUDF.codegen.enabled=false +-- Config: spark.sql.subexpressionElimination.enabled=true + +statement +CREATE TABLE make_date_overflow(y INT, m INT, d INT) USING parquet + +statement +INSERT INTO make_date_overflow VALUES (6000000, 1, 1) + +-- Java accepts the calendar date; its epoch day does not fit Date32, even without ANSI. +query expect_error_class(ARITHMETIC_OVERFLOW) +SELECT make_date(y, m, d) FROM make_date_overflow + +statement +CREATE TABLE make_date_boundaries(y INT, m INT, d INT) USING parquet + +statement +INSERT INTO make_date_boundaries VALUES + (5881580, 7, 11), (-5877641, 6, 23), + (NULL, 13, 32), (6000000, NULL, 1), (6000000, 1, NULL) + +query expect_native(make_date) +SELECT unix_date(make_date(y, m, d)) FROM make_date_boundaries + +-- Container constructors evaluate every child and do not need a parent fallback. +query expect_native(make_date) +SELECT array(make_date(y, m, d), DATE '2024-01-01'), Review Comment: [P2] Enable Java 8 date decoding before collecting these boundary dates. The fixture contains `(-5877641, 6, 23)`, which produces `Int.MinValue` epoch days. With the default date API, collecting the array/struct invokes Spark's `toJavaDate`, whose Gregorian-to-Julian rebasing overflows. Both ANSI variants therefore fail before reaching the remaining regressions. Add `-- Config: spark.sql.datetime.java8API.enabled=true` so the test compares valid `LocalDate` values. Evidence: Running the original fixture through `CometSqlFileTestSuite` on Spark 4.1.3/JDK 21 failed in both ANSI modes with `EXPRESSION_DECODING_FAILED`, caused by `ArithmeticException: integer overflow` in `RebaseDateTime.localRebaseGregorianToJulianDays` via `DateTimeUtils.toJavaDate`. A disposable copy with the Java 8 date setting passed this query. ########## spark/src/test/scala/org/apache/comet/rules/CometExecRuleSuite.scala: ########## @@ -125,6 +127,151 @@ class CometExecRuleSuite extends CometTestBase { }.sum } + test("make_date preserves row demand below lazy consumers, including reused native plans") { + withSQLConf( + SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false", + CometConf.COMET_EXEC_LOCAL_TABLE_SCAN_ENABLED.key -> "true") { + val input = createSparkPlan(spark, "SELECT * FROM VALUES (2024), (6000000) AS t(y)") + val consumers: Seq[(String, SparkPlan => SparkPlan)] = Seq( + ("LIMIT", child => CollectLimitExec(1, child)), + ("LIMIT", child => LocalLimitExec(1, child)), + ("LIMIT", child => GlobalLimitExec(1, child)), + ("SAMPLE", child => SampleExec(0.0, 0.5, false, 42L, child)), Review Comment: [P2] Make this `SampleExec` construction compatible with Spark 4.2. Its seed parameter is `Option[Long]`, whereas earlier supported versions accept `Long`. Passing `42L` directly from shared test code prevents the Spark 4.2 test sources from compiling. Use a version-specific helper or obtain the sampling operator from Spark's planner so every supported profile can compile this regression. Evidence: Exact-head CI job https://github.com/apache/datafusion-comet/actions/runs/39293853592/job/118204860246 reports `CometExecRuleSuite.scala:139: type mismatch; found: Long(42L), required: Option[Long]`. The Spark 4.2 source confirms `SampleExec(..., seed: Option[Long], child: SparkPlan)`. ########## spark/src/test/resources/sql-tests/expressions/datetime/make_date_overflow.sql: ########## @@ -0,0 +1,109 @@ +-- 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. + +-- ConfigMatrix: spark.sql.ansi.enabled=false,true +-- Config: spark.comet.exec.scalaUDF.codegen.enabled=false +-- Config: spark.sql.subexpressionElimination.enabled=true + +statement +CREATE TABLE make_date_overflow(y INT, m INT, d INT) USING parquet + +statement +INSERT INTO make_date_overflow VALUES (6000000, 1, 1) + +-- Java accepts the calendar date; its epoch day does not fit Date32, even without ANSI. +query expect_error_class(ARITHMETIC_OVERFLOW) +SELECT make_date(y, m, d) FROM make_date_overflow + +statement +CREATE TABLE make_date_boundaries(y INT, m INT, d INT) USING parquet + +statement +INSERT INTO make_date_boundaries VALUES + (5881580, 7, 11), (-5877641, 6, 23), + (NULL, 13, 32), (6000000, NULL, 1), (6000000, 1, NULL) + +query expect_native(make_date) +SELECT unix_date(make_date(y, m, d)) FROM make_date_boundaries + +-- Container constructors evaluate every child and do not need a parent fallback. +query expect_native(make_date) +SELECT array(make_date(y, m, d), DATE '2024-01-01'), + named_struct('date', make_date(y, m, d)) FROM make_date_boundaries + +statement +CREATE TABLE make_date_arguments(y INT, m INT, inner_y INT) USING parquet + +statement +INSERT INTO make_date_arguments VALUES + (NULL, 1, 6000000), (2024, NULL, 6000000), (2024, 1, 2024) + +-- Year and month NULLs skip the overflowing day expression on just those rows. +query expect_native(make_date) +SELECT make_date(y, m, dayofmonth(make_date(inner_y, 1, 1))) FROM make_date_arguments + +-- A NULL year also skips a computed month, before it can overflow. +query expect_native(make_date) +SELECT make_date(y, month(make_date(inner_y, 1, 1)), 1) +FROM make_date_arguments WHERE m IS NOT NULL + +-- A later NULL must not hide an error in the required first argument. +query expect_error_class(ARITHMETIC_OVERFLOW) +SELECT make_date(year(make_date(inner_y, 1, 1)), 1, y) Review Comment: [P2] Restrict this overflow assertion to rows that produce the intended error. The `(2024, 1, 2024)` input also reaches this query and becomes `make_date(2024, 1, 2024)`, which raises an invalid-day exception under ANSI. Other rows raise arithmetic overflow, so scan/task order can select different exception classes in Spark and Comet and fail the fixture despite both rejecting the input. Filtering this assertion with `WHERE inner_y = 6000000` preserves the intended evaluation-order check without the competing error. Evidence: After enabling Java 8 date decoding in the disposable fixture, two consecutive runs failed the ANSI variant here with `SparkDateTimeException did not equal SparkArithmeticException`. Adding only `WHERE inner_y = 6000000` to this query then made both ANSI fixture variants pass. ########## spark/src/test/scala/org/apache/comet/CometTemporalExpressionSuite.scala: ########## @@ -56,6 +58,52 @@ class CometTemporalExpressionSuite extends CometTestBase with AdaptiveSparkPlanH .getOrElse( fail(s"No SparkThrowable in cause chain: ${causeChain(error).map(_.getClass.getName)}")) + test("make_date does not evaluate an overflowing row beyond LIMIT") { + withSQLConf( + SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false", + SQLConf.OPTIMIZER_EXCLUDED_RULES.key -> + "org.apache.spark.sql.catalyst.optimizer.PushProjectionThroughLimit", + CometConf.COMET_BATCH_SIZE.key -> "8192") { + withTempPath { path => + withSQLConf(CometConf.COMET_ENABLED.key -> "false") { + spark + .range(2) + .coalesce(1) + .selectExpr("CAST(CASE WHEN id = 0 THEN 2024 ELSE 6000000 END AS INT) AS y") + .write + .parquet(path.getCanonicalPath) + } + withTempView("make_date_limit") { + spark.read.parquet(path.getCanonicalPath).createOrReplaceTempView("make_date_limit") + for (ansi <- Seq(false, true); codegen <- Seq(false, true)) { + withSQLConf( + SQLConf.ANSI_ENABLED.key -> ansi.toString, + SQLConf.WHOLESTAGE_CODEGEN_ENABLED.key -> codegen.toString) { + val query = "SELECT unix_date(make_date(y, 1, 1)) FROM make_date_limit LIMIT 1" + val expected = withSQLConf(CometConf.COMET_ENABLED.key -> "false") { Review Comment: [P2] Capture the expected rows without relying on `withSQLConf` returning them. In Spark 3.4 and 3.5, `SQLTestUtils.withSQLConf` returns `Unit`, so `expected` becomes `()` and the following comparison with `Seq(Row(19723))` always fails. This already breaks the Spark 3.5 strict-warnings build and would fail the regression test without strict warnings. Assign the collected result inside the block to a separately declared value, or use a cross-version helper. Evidence: Exact-head CI job https://github.com/apache/datafusion-comet/actions/runs/39293853592/job/118204860318 fails at line 86: `comparing values of types Unit and Seq[org.apache.spark.sql.Row] using == will always yield false`. Spark 3.4.3 and 3.5.9 sources declare `withSQLConf(...)(f: => Unit): Unit`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
