andygrove commented on code in PR #5622: URL: https://github.com/apache/datafusion-comet/pull/5622#discussion_r3941100587
########## spark/src/test/resources/sql-tests/expressions/math/abs.sql: ########## @@ -27,3 +27,25 @@ SELECT abs(i), abs(l), abs(f), abs(d) FROM test_abs -- literal arguments query SELECT abs(-5), abs(-1.5), abs(0), abs(NULL) + +-- abs() on intervals has no native impl; routed through the JVM codegen dispatcher. +-- Interval values are built inline: native Parquet scan of interval columns is unsupported +-- (https://github.com/apache/datafusion-comet/issues/5060), and a top-level YearMonthIntervalType projection column is still rejected by the +-- projection type gate (https://github.com/apache/datafusion-comet/issues/5061), so the ym result is wrapped in a struct. +query +SELECT abs(make_dt_interval(1, 2, 3, 4.5)) AS dt_pos, + abs(make_dt_interval(-1, -2, -3, -4.5)) AS dt_neg, + abs(make_dt_interval(0, 0, 0, 0)) AS dt_zero, + abs(CAST(NULL AS INTERVAL DAY TO SECOND)) AS dt_null Review Comment: All four interval queries in this file use compile-time constants, so every row sees the same input. Excluding `ConstantFolding` does make the kernel run per row, but a bug in row indexing, the null mask, or a mid-batch overflow would still not be caught. #5060 does block reading an interval column, but you can vary the value per row by deriving it from an integer column, which is also the shape a real query has. I ran these against the PR and they pass, so this is coverage rather than a bug: ```sql statement CREATE TABLE test_abs_iv(d int, h int, m int, s decimal(8,6)) USING parquet statement INSERT INTO test_abs_iv VALUES (1, 2, 3, 4.5), (-1, -2, -3, -4.5), (0, 0, 0, 0), (NULL, 0, 0, 0), (5, -1, 30, -0.000001) query SELECT abs(make_dt_interval(d, h, m, s)) FROM test_abs_iv ORDER BY d ``` A mid-batch overflow case is worth adding on the same table. I checked that one `Long.MinValue` row among valid rows raises the same message Spark does, but nothing here asserts it, since the existing overflow queries are single-row constants. #5061 calls this trap out directly: "a single-row `VALUES (make_dt_interval(...))` gets collapsed by `ConvertToLocalRelation` into a folded literal ... Derive intervals from Parquet-backed integer columns instead.". ########## spark/src/main/scala/org/apache/comet/serde/math.scala: ########## @@ -169,9 +169,11 @@ object CometUnhex extends CometExpressionSerde[Unhex] with MathExprBase { } } -object CometAbs extends CometExpressionSerde[Abs] with MathExprBase { +object CometAbs extends CometExpressionSerde[Abs] with MathExprBase with CodegenDispatchFallback { - val unsupportedReason: String = "Only integral, floating-point, and decimal types are supported" + val unsupportedReason: String = + "Interval types are not supported natively and are handled via JVM codegen dispatch; " + + "this fallback only applies when the dispatcher is disabled" Review Comment: This string does double duty as the `Unsupported` note that shows up in EXPLAIN and as the `getUnsupportedReasons()` bullet rendered into the generated compat docs, and I think it reads awkwardly in both. `GenerateDocs.writeExpressionCompatNotes` already prints the mechanism as a header for any `CodegenDispatchFallback` serde, so the generated `math.md` says the same thing twice: ``` The following cases have no native implementation and always run in the JVM using Spark's code-generated implementation (inside the Comet pipeline): - Interval types are not supported natively and are handled via JVM codegen dispatch; this fallback only applies when the dispatcher is disabled ``` And in EXPLAIN with the dispatcher off it contradicts the reason sitting next to it. This is what I get with `spark.comet.exec.scalaUDF.codegen.enabled=false`: ``` Project [COMET: abs: spark.comet.exec.scalaUDF.codegen.enabled=false; expression has no native path so the plan falls back to Spark, Interval types are not supported natively and are handled via JVM codegen dispatch; this fallback only applies when the dispatcher is disabled] ``` `CometRound` in `arithmetic.scala` has exactly this shape and keeps the two concerns apart. The reasons name only the case, and a class-level scaladoc explains the dispatch. Would you follow that here? ```scala /** * `abs` lowers to the native `abs` kernel for numeric inputs. Interval inputs have no native * implementation, so `CodegenDispatchFallback` keeps them in the Comet pipeline by running * Spark's own `Abs.doGenCode` in the JVM codegen dispatcher, which matches Spark exactly. */ object CometAbs extends CometExpressionSerde[Abs] with MathExprBase with CodegenDispatchFallback { val unsupportedReason: String = "`INTERVAL YEAR TO MONTH` and `INTERVAL DAY TO SECOND` inputs" ``` ########## spark/src/test/resources/sql-tests/expressions/math/abs_ansi.sql: ########## @@ -104,3 +104,9 @@ SELECT abs(v) FROM ansi_test_abs_byte -- literal query expect_error(overflow) SELECT abs(cast(-128 as tinyint)) + +query expect_error(overflow) +SELECT abs(make_dt_interval(-106751991, -4, 0, -54.775808)) + +query expect_error(overflow) +SELECT abs(make_ym_interval(0, -2147483648)) Review Comment: This file carries `MaxSparkVersion: 4.1`, so on the Spark 4.2 CI job it is skipped and `abs_ansi_spark42.sql` runs instead. Because these two cases were only added here, 4.2 and later get no ANSI interval coverage at all. Every other case in that pair exists in both files. Could you mirror these two into `abs_ansi_spark42.sql` with the loose `overflow` pattern it already uses? One thing that might save you work. Spark's `Abs.doGenCode` sends `_: AnsiIntervalType` to `MathUtils.negateExact` unconditionally, with no `failOnError` check, so interval overflow behavior is identical in both ANSI modes and the `abs.sql` cases already assert it. Keeping these ANSI copies to pin that independence is fine by me, but then they belong in both ANSI fixtures rather than one. ########## spark/src/test/resources/sql-tests/expressions/math/abs.sql: ########## @@ -27,3 +27,25 @@ SELECT abs(i), abs(l), abs(f), abs(d) FROM test_abs -- literal arguments query SELECT abs(-5), abs(-1.5), abs(0), abs(NULL) + +-- abs() on intervals has no native impl; routed through the JVM codegen dispatcher. +-- Interval values are built inline: native Parquet scan of interval columns is unsupported +-- (https://github.com/apache/datafusion-comet/issues/5060), and a top-level YearMonthIntervalType projection column is still rejected by the +-- projection type gate (https://github.com/apache/datafusion-comet/issues/5061), so the ym result is wrapped in a struct. +query +SELECT abs(make_dt_interval(1, 2, 3, 4.5)) AS dt_pos, + abs(make_dt_interval(-1, -2, -3, -4.5)) AS dt_neg, + abs(make_dt_interval(0, 0, 0, 0)) AS dt_zero, + abs(CAST(NULL AS INTERVAL DAY TO SECOND)) AS dt_null + +-- interval year to month: dispatched the same way; wrapped in a struct because a top-level +-- YearMonthIntervalType column is rejected by the projection output type gate (https://github.com/apache/datafusion-comet/issues/5061) +query +SELECT named_struct('v', abs(make_ym_interval(1, 6))) AS ym_pos, + named_struct('v', abs(make_ym_interval(-1, -6))) AS ym_neg Review Comment: I do not think this wrapper is needed. I ran the query verbatim with `named_struct` removed, under a strict `checkSparkAnswerAndOperator`, and it stays fully native both over `OneRowRelation` and over a Parquet-backed table: ```sql SELECT abs(make_ym_interval(1, 6)) AS ym_pos, abs(make_ym_interval(-1, -6)) AS ym_neg ``` There is no projection output-type gate for `YearMonthIntervalType`. `QueryPlanSerde.supportedDataType` does reject both ANSI interval types, but nothing consults it for a projection's output. Its callers are `CometLiteral`, `CometSink`, `CometScalarSubquery` and `hash.scala`, and `CometSink.supportedSinkDataType` admits both interval types explicitly. The wrapper also costs coverage. With it, extended explain reports `1 native, 1 codegen dispatch`, where the native expression is the `named_struct` itself. The interval value never becomes a top-level output column, so the `IntervalYearVector` output path is never exercised. Without the wrapper you get `0 native, 1 codegen dispatch` and that path is covered. Here is what I measured, all with `ConstantFolding` excluded and ANSI off: | query | fully native | |---|---| | `SELECT abs(make_ym_interval(1, 6))` | yes | | `SELECT abs(make_ym_interval(1, 6)), abs(make_ym_interval(-1, -6))` | yes | | `SELECT abs(make_ym_interval(y, m)) FROM t`, Parquet, 5 rows including a null | yes | | `SELECT abs(CAST(NULL AS INTERVAL YEAR TO MONTH))` | no | | `SELECT CAST(NULL AS INTERVAL YEAR TO MONTH)` | no | Those last two rows are what I suspect you actually ran into, and it is a different problem. See my next comment. Could you drop the wrapper and both `#5061` references? Worth noting that `#5061` is the interval EPIC rather than an issue about a projection gate, so the link is misleading on its own even setting the mechanism aside. ########## spark/src/test/resources/sql-tests/expressions/math/abs.sql: ########## @@ -27,3 +27,25 @@ SELECT abs(i), abs(l), abs(f), abs(d) FROM test_abs -- literal arguments query SELECT abs(-5), abs(-1.5), abs(0), abs(NULL) + +-- abs() on intervals has no native impl; routed through the JVM codegen dispatcher. +-- Interval values are built inline: native Parquet scan of interval columns is unsupported +-- (https://github.com/apache/datafusion-comet/issues/5060), and a top-level YearMonthIntervalType projection column is still rejected by the +-- projection type gate (https://github.com/apache/datafusion-comet/issues/5061), so the ym result is wrapped in a struct. +query +SELECT abs(make_dt_interval(1, 2, 3, 4.5)) AS dt_pos, + abs(make_dt_interval(-1, -2, -3, -4.5)) AS dt_neg, + abs(make_dt_interval(0, 0, 0, 0)) AS dt_zero, + abs(CAST(NULL AS INTERVAL DAY TO SECOND)) AS dt_null + +-- interval year to month: dispatched the same way; wrapped in a struct because a top-level +-- YearMonthIntervalType column is rejected by the projection output type gate (https://github.com/apache/datafusion-comet/issues/5061) +query +SELECT named_struct('v', abs(make_ym_interval(1, 6))) AS ym_pos, + named_struct('v', abs(make_ym_interval(-1, -6))) AS ym_neg Review Comment: Separately from the wrapper, this query has no null case while the `dt` query above has `dt_null`. I would add one, because it turns up a real gap rather than just passing: ``` Project [COMET: Unsupported data type YearMonthIntervalType(0,1)] ``` `NullPropagation` folds `abs(CAST(NULL AS INTERVAL YEAR TO MONTH))` down to a bare `Literal(null, YearMonthIntervalType)`, and `CometLiteral.getSupportLevel` special-cases only `DayTimeIntervalType` (`literals.scala:63`), so the literal is `Unsupported`. `CometLiteral` does not mix in `CodegenDispatchFallback`, so there is no dispatch rescue and the whole projection falls back. This is pre-existing rather than anything you introduced. `SELECT CAST(NULL AS INTERVAL YEAR TO MONTH)` falls back the same way with no `abs` in the query at all, and it is already a bullet in #5061. But this PR is what makes it reachable through `abs`, so I would rather pin it than have it silently avoided: ```sql query expect_fallback(Unsupported data type YearMonthIntervalType) SELECT abs(CAST(NULL AS INTERVAL YEAR TO MONTH)) ``` That way it flips to a test failure when the literal gap gets fixed. -- 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]
