You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "0lai0 (via GitHub)" <gi...@apache.org> on 2026/07/27 14:26:15 UTC
[PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
0lai0 opened a new pull request, #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #5013
## Rationale for this change
With `spark.sql.legacy.allowNegativeScaleOfDecimal=true`, Spark allows `DecimalType(p, s)` with `s < 0`. Comet's native cast and decimal arithmetic kernels scale-align values by computing `10^|scale|` (e.g. `10_i128.pow(scale as u32)`). When `scale` is negative, casting it to `u32` produces a huge value, so the power computation overflows: it panics (`attempt to multiply/subtract with overflow`) in debug builds and silently wraps to an incorrect value in release builds (the native workspace sets `overflow-checks = false` for release).
This is reachable from two places:
- Casting an integer type (`Byte`/`Short`/`Integer`/`Long`) to/from a negative-scale `DecimalType`, or casting a negative-scale `DecimalType` to `TimestampType`, both hit the same `10^|scale|` computation in `numeric.rs` / `cast_decimal_to_timestamp`.
- Arithmetic on negative-scale decimals (`Add`/`Subtract`/`Divide`/`IntegralDivide`/`Remainder`), which scale-aligns operands the same way.
`CometCast` already special-cased negative-scale decimals for the `Decimal -> String` direction, but that guard was unreachable in practice because producing the negative-scale value in the first place would panic before a `-> String` cast could ever run.
Since `allowNegativeScaleOfDecimal` is a rarely-used legacy flag (default `false`), and the native kernels don't implement negative-scale-safe rescaling, the fix makes Comet detect these cases at planning time and fall back to Spark instead of attempting them natively.
## What changes are included in this PR?
- `CometCast.scala`: added `negativeScaleDecimalCastReason` and marked as `Unsupported`:
- `int -> Decimal(neg)` via `canCastFromByte`/`Short`/`Int`/`Long`
- `Decimal(neg) -> int` and `Decimal(neg) -> Timestamp` via `canCastFromDecimal` (now takes the source `DecimalType` so it can check `fromType.scale`)
All other cast directions to/from negative-scale decimals (e.g. `String <-> Decimal(neg)`, `Decimal(neg) -> Float/Double/String/Boolean/wider-Decimal`) don't perform integer scale-alignment and are unaffected, they keep running natively.
- `arithmetic.scala`: added `negScaleDecimalRejection(expr: BinaryArithmetic)` in `MathBase`, returning `Unsupported` when the left operand, right operand, or result type is a negative-scale `DecimalType`. Wired into `CometAdd`, `CometSubtract`, `CometDivide`, `CometIntegralDivide`, `CometRemainder`.
`CometMultiply` and `UnaryMinus` are intentionally left unguarded, neither scale-aligns, so they stay native
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Added tests to `CometCastSuite` and `CometExpressionSuite`
`make core`
` ./mvnw -pl spark test -Dsuites='org.apache.comet.CometCastSuite'`
`./mvnw -pl spark test -Dsuites='org.apache.comet.CometExpressionSuite'`
**Note**: the `TimestampType` case is not called out in the linked issue's reproducer, it was found while auditing the same `10^|scale|` code path during this fix and is included here since it shares the exact same root cause and fix.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-5092878285
This may overlap with https://github.com/apache/datafusion-comet/pull/4799
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-5204367900
One suggestion: `QueryPlanSerde.exprToProtoInternal` is a single funnel that every expression passes through, and it recurses into children, so a guard there on `expr.dataType` covers everything in one place — `Cast(Decimal(10,-2) as bigint)` is caught because the child attribute's own type gets checked on recursion, and the arithmetic cases fall out the same way.
I'd build it on `SupportLevel.containsType`, which already walks array/map/struct at every nesting level, so nested negative-scale decimals come for free. A small helper next to `SupportLevel.strictFloatingPointReason` (same shape, returning `Option[String]`) called from the top of `exprToProtoInternal` would do it, and it keeps per-expression granularity plus the normal `withFallbackReason` path so the reason shows up in extended explain.
Nothing is needed on the scan side — the Parquet spec requires `scale >= 0`, so negative-scale decimals can only arise from expressions.
The tests you've added are the valuable part here and will cover this just as well.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-6196059368
@0lai0 are you still planning to finish this? d8b0a060d6 looks like it covers the code changes from the last round, but a couple of things are still open. comphead's question on `arithmetic.scala` hasn't had a reply, and the description still lists the float, double, add, subtract and remainder guards that commit removed. Would you like to pick those up, or would you be ok with me continuing the work on this branch? I'd like to get it into the 1.2.0 release.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-5152092657
This is a small targeted PR that seems ready for review and #4799 is still in flux. Let's reopen this one.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-5152333021
@andygrove @0lai0 WDYT instead of listing ops if we scan an entire input plan and fallback if there is negative decimals?
Something like
```
object NegativeScaleDecimalFinder {
/** True if `dt` is (or nests) a `DecimalType` with negative scale. */
def hasNegativeScaleDecimal(dt: DataType): Boolean = dt match {
case d: DecimalType => d.scale < 0
case ArrayType(elem, _) => hasNegativeScaleDecimal(elem)
case MapType(k, v, _) => hasNegativeScaleDecimal(k) || hasNegativeScaleDecimal(v)
case s: StructType => s.fields.exists(f => hasNegativeScaleDecimal(f.dataType))
case _ => false
}
/** True if any expression in the plan (or its subqueries) produces a negative-scale decimal. */
def planHasNegativeScaleDecimal(plan: QueryPlan[_]): Boolean = {
def exprHas(e: Expression): Boolean =
hasNegativeScaleDecimal(e.dataType) || e.children.exists(exprHas)
plan.exists { node =>
node.output.exists(a => hasNegativeScaleDecimal(a.dataType)) ||
node.expressions.exists(exprHas) ||
node.subqueries.exists(planHasNegativeScaleDecimal)
}
}
/** Returns the offending nodes for easier debugging. */
def findNegativeScaleDecimalNodes(plan: QueryPlan[_]): Seq[QueryPlan[_]] = {
plan.collect {
case node if node.output.exists(a => hasNegativeScaleDecimal(a.dataType)) ||
node.expressions.exists(e =>
hasNegativeScaleDecimal(e.dataType) ||
e.find(x => hasNegativeScaleDecimal(x.dataType)).isDefined) =>
node
}
}
}
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-6203234050
@andygrove yes, finishing it, thanks for the nudge. Replied to @comphead's two questions and rewrote the description, which was still listing guards that have since been removed. 1.2.0 works for me.
@sunchao thanks, precise catch.
**The P2** is fixed with your first option. The reader now takes the unscaled-long path only when `precision <= 18 && scale >= 0`, at all three selection points plus the runtime branch, so a negative scale gets the `BigDecimal`-backed reader.
Two notes from reproducing it. First, it is wider than TRY: plain `cast` failed the query rather than returning NULL.
```
try_cast(v as int) comet=[NULL,NULL,NULL] spark=[100,-100,0]
cast(v as int) comet=[THREW SparkException] spark=[100,-100,0]
```
Second, the reader bug predates this PR. It is reachable on `main` with no cast guard involved, through any dispatched expression converting a negative-scale decimal to an integral type. A `ScalaUDF` with an `Int` parameter over a materialized `Decimal(10,-1)` column does it. Happy to split the reader fix into its own PR if you would prefer.
The other two items are done: the materialized-input regression excludes `CollapseProject` so the decimal really is an input, and the float/double guard is removed since #5684 made those exact for negative scale.
**Validation**: debug build, clippy clean, nine suites green with 600 passing and 0 failures, including four codegen suites because the reader change touches every dispatched decimal read.
I have not run the full Spark SQL suite locally. This touches the serde, the planner and the dispatcher, so per the CI guide `run-spark-4.1-tests` is the one that would catch a behavior difference. I cannot add labels myself, so could one of you apply it?
**One question back**: you noted the native TRY cast is already correct, which I confirmed, while LEGACY and ANSI still fail. That means the guard is scoped too widely and could skip TRY entirely rather than routing it through the dispatcher. Threading `evalMode` into `canCastFromDecimal` plus checking the reverse direction is more than I wanted to add here. Worth doing in this PR, or a follow-up?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4307692004
##########
docs/source/user-guide/latest/compatibility/expressions/_category_template/cast.md:
##########
@@ -165,6 +165,22 @@ Spark's behavior of using Java `BigDecimal.toString()` semantics, which produces
notation (e.g. a value of 12300 stored as `Decimal(7,-2)` with unscaled value 123 is rendered
as `"1.23E+4"`).
+## Decimal with Negative Scale to Integer and Timestamp
+
+Casting a `DecimalType` with a negative scale to `ByteType`, `ShortType`, `IntegerType`,
+`LongType` or `TimestampType` has no native implementation, and neither does casting any of the
+integer types to a negative-scale `DecimalType`. Comet reports these as unsupported, which
+routes them through the JVM codegen dispatcher (Spark's own generated code) inside the Comet
+pipeline, so the results match Spark exactly.
+
+The native kernels align scale by computing `10^|scale|` in fixed-point arithmetic, which
+overflows when the scale is negative. See
+[#5013](https://github.com/apache/datafusion-comet/issues/5013).
+
+All other cast directions to and from negative-scale decimals run natively: `FloatType` and
+`DoubleType`, `StringType` in both directions (subject to the config gate described above),
+`BooleanType`, and widening to another `DecimalType`.
Review Comment:
Fixed. It now says casts from a negative-scale decimal to `BooleanType`, and adds that the other direction goes through the dispatcher at any scale, since that is not specific to a negative one.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r3696042534
##########
spark/src/main/scala/org/apache/comet/serde/arithmetic.scala:
##########
@@ -91,6 +90,25 @@ trait MathBase {
Unsupported(Some(s"Unsupported datatype $dt"))
}
+ // Native decimal Add/Subtract/Divide/IntegralDivide/Remainder scale-aligns operands by
+ // multiplying by 10^|delta|, which overflows (subtract-with-overflow panic in debug, silent
+ // wrap in release) whenever any operand or the result has negative scale. See issue #5013.
+ private[comet] val negScaleDecimalArithmeticReason: String =
+ "Arithmetic on negative-scale decimal is not supported natively"
+
+ private[comet] def negScaleDecimalRejection(expr: BinaryArithmetic): Option[Unsupported] = {
Review Comment:
do we really need changes in this file?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4307675772
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1486,6 +1487,111 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
+ test("division on negative-scale decimal reports unsupported via getSupportLevel") {
+ // Issue #5013: `spark_decimal_div` computes its scale-alignment exponents in `u32`, so a
+ // negative scale wraps to roughly 4.29e9 and the BigInt branch attempts an unbounded
+ // allocation. Only division is affected. The other arithmetic operators scale-align by
+ // `10^(max_scale - s)`, which is never negative, so they stay native and are pinned by
+ // "safe ops on negative-scale decimal run natively" below. Constructing the expressions
+ // requires allowNegativeScaleOfDecimal so DecimalType(_, s<0) passes Spark's own check.
+ withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true") {
+ Seq(DecimalType(10, -1), DecimalType(20, -5)).foreach { negScale =>
+ val posDec = DecimalType(10, 0)
+ val negAttr = AttributeReference("n", negScale)()
+ val posAttr = AttributeReference("p", posDec)()
+ val expected = Unsupported(Some(CometDivide.negScaleDecimalArithmeticReason))
+
+ assert(CometDivide.getSupportLevel(Divide(negAttr, posAttr)) == expected)
+ assert(CometIntegralDivide.getSupportLevel(IntegralDivide(negAttr, posAttr)) == expected)
+ // Guard also fires when negative scale is only on the right operand.
+ assert(CometDivide.getSupportLevel(Divide(posAttr, negAttr)) == expected)
+
+ // The unguarded operators must not report unsupported for the same operands.
+ assert(CometAdd.getSupportLevel(Add(negAttr, posAttr)) == Compatible())
+ assert(CometSubtract.getSupportLevel(Subtract(negAttr, posAttr)) == Compatible())
+ assert(CometRemainder.getSupportLevel(Remainder(negAttr, posAttr)) == Compatible())
+ assert(CometMultiply.getSupportLevel(Multiply(negAttr, posAttr)) == Compatible())
+ }
+ }
+ }
+
+ test("division on negative-scale decimal falls back and returns correct results") {
+ // End-to-end proof for issue #5013 that the previously-panicking queries now complete.
+ // `checkSparkAnswerAndFallbackReason` rather than `checkSparkAnswer`: these five serdes do
+ // not mix in `CodegenDispatchFallback`, so `Unsupported` falls the whole projection back to
+ // Spark, and asserting the reason is what proves the guard fired. A plain answer comparison
+ // would pass whether or not it did.
+ // ConvertToLocalRelation is excluded so the arithmetic actually runs on the plan rather
+ // than being folded at plan time (#4789). `DecimalType(_, s<0)` must be constructed under
+ // allowNegativeScaleOfDecimal=true because the case class initializer reads the flag, so
+ // wrap everything (including the type value) in one block.
+ withSQLConf(
+ "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true",
+ CometConf.COMET_EXEC_LOCAL_TABLE_SCAN_ENABLED.key -> "true",
+ SQLConf.OPTIMIZER_EXCLUDED_RULES.key ->
+ "org.apache.spark.sql.catalyst.optimizer.ConvertToLocalRelation") {
+ // Cover a narrow and a wide negative-scale type: at precision 20 the combined operand
+ // width crosses DECIMAL128_MAX_PRECISION, which routes the operation to
+ // `WideDecimalBinaryExpr` / `decimal_div`'s BigInt path rather than the narrow arrow
+ // kernels. `unit` is the smallest magnitude representable at that scale.
+ val negScaleCases = Seq((DecimalType(10, -1), 10), (DecimalType(20, -5), 100000))
+ negScaleCases.foreach { case (negScaleType, unit) =>
+ // Build the neg-scale column via string -> Decimal(neg), a known-safe cast, so the
+ // source construction does not depend on any of the guards under test.
+ val strs = Seq(10 * unit, 20 * unit, 30 * unit).map(_.toString)
+ val v = strs.toDF("s").select(col("s").cast(negScaleType).as("v"))
+ Seq("v / cast(3 as decimal(10, 0))", "v div cast(3 as decimal(10, 0))").foreach { e =>
+ checkSparkAnswerAndFallbackReason(
+ v.selectExpr(e),
+ CometDivide.negScaleDecimalArithmeticReason)
+ }
+ }
+ }
+ }
+
+ test("safe ops on negative-scale decimal run natively (regression pin)") {
+ // Multiply and UnaryMinus on negative-scale decimals don't scale-align an integer and
+ // ran natively at the time #5013 was fixed. The arithmetic guard intentionally does
+ // not cover them. If a future change ever adds scale-alignment to Multiply or a
+ // decimal codepath is added to UnaryMinus that does the multiply-by-10^n trick,
+ // these assertions will start returning a Comet fallback plan and this test will
+ // catch it before it reaches production as a silent-wrap or debug panic.
+ withSQLConf(
+ "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true",
+ CometConf.COMET_EXEC_LOCAL_TABLE_SCAN_ENABLED.key -> "true",
+ SQLConf.OPTIMIZER_EXCLUDED_RULES.key ->
+ "org.apache.spark.sql.catalyst.optimizer.ConvertToLocalRelation") {
+ // The wide case matters most for `v * v`: Decimal(20,-5) * Decimal(20,-5) has
+ // p1 + p2 >= DECIMAL128_MAX_PRECISION, so it takes `WideDecimalBinaryExpr`'s multiply
+ // branch (which does compute a scale adjustment against the output precision/scale)
+ // and its result type is Decimal(38,-10) -- a negative *output* scale, not just
+ // negative inputs.
+ val negScaleCases = Seq((DecimalType(10, -1), 10), (DecimalType(20, -5), 100000))
+ negScaleCases.foreach { case (negScaleType, unit) =>
+ val strs = Seq(10 * unit, 20 * unit, 30 * unit).map(_.toString)
+ val v = strs.toDF("s").select(col("s").cast(negScaleType).as("v"))
+ // Same-scale Multiply - no alignment needed, must stay native.
+ checkSparkAnswerAndOperator(v.selectExpr("v * v"))
+ checkSparkAnswerAndOperator(v.selectExpr("v * cast(2 as int)"))
+ // UnaryMinus on Decimal(neg) - no alignment, must stay native.
+ checkSparkAnswerAndOperator(v.selectExpr("-v"))
+ // Add, Subtract and Remainder scale-align by `10^(max_scale - s)`, which is never
+ // negative. They were guarded in an earlier revision of this PR because the `u8`
+ // underflow in the two width tests made them fail. Those are rewritten in `i16` here,
+ // so they run natively again. If either rewrite is reverted these assertions fail.
Review Comment:
You are right that the Scala assertions cannot pin the rewrites with `overflow-checks = false`. I have reworded that comment rather than leaving it claiming something it cannot do.
Three tests added, which do run with overflow checks on:
- `test_decimal128_add_sub_width_handles_negative_scale`, covering the negative-scale cases plus a few non-negative ones to pin the unchanged behavior
- `test_modulo_decimal_negative_scale`, next to `test_modulo_basic_decimal`
- `test_decimal_div_rejects_negative_scale`, next to `test_decimal_divide_by_zero_returns_spark_error`, covering all three positions a negative scale can appear in
On sharing the calculation, I did not do it quite as suggested, because the two sites use different boundaries: `create_binary_expr_with_options` tests `>= DECIMAL128_MAX_PRECISION` and `create_modulo_expr` tests `>`. A shared predicate would have changed one of them. So the shared piece is the calculation, `decimal128_add_sub_width(p1, s1, p2, s2) -> i16`, and each site keeps its own comparison. That still leaves one copy of the expression and makes it testable.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4307693702
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1486,6 +1487,111 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
+ test("division on negative-scale decimal reports unsupported via getSupportLevel") {
+ // Issue #5013: `spark_decimal_div` computes its scale-alignment exponents in `u32`, so a
+ // negative scale wraps to roughly 4.29e9 and the BigInt branch attempts an unbounded
+ // allocation. Only division is affected. The other arithmetic operators scale-align by
+ // `10^(max_scale - s)`, which is never negative, so they stay native and are pinned by
+ // "safe ops on negative-scale decimal run natively" below. Constructing the expressions
+ // requires allowNegativeScaleOfDecimal so DecimalType(_, s<0) passes Spark's own check.
+ withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true") {
+ Seq(DecimalType(10, -1), DecimalType(20, -5)).foreach { negScale =>
+ val posDec = DecimalType(10, 0)
+ val negAttr = AttributeReference("n", negScale)()
+ val posAttr = AttributeReference("p", posDec)()
+ val expected = Unsupported(Some(CometDivide.negScaleDecimalArithmeticReason))
+
+ assert(CometDivide.getSupportLevel(Divide(negAttr, posAttr)) == expected)
+ assert(CometIntegralDivide.getSupportLevel(IntegralDivide(negAttr, posAttr)) == expected)
+ // Guard also fires when negative scale is only on the right operand.
+ assert(CometDivide.getSupportLevel(Divide(posAttr, negAttr)) == expected)
+
+ // The unguarded operators must not report unsupported for the same operands.
+ assert(CometAdd.getSupportLevel(Add(negAttr, posAttr)) == Compatible())
+ assert(CometSubtract.getSupportLevel(Subtract(negAttr, posAttr)) == Compatible())
+ assert(CometRemainder.getSupportLevel(Remainder(negAttr, posAttr)) == Compatible())
+ assert(CometMultiply.getSupportLevel(Multiply(negAttr, posAttr)) == Compatible())
+ }
+ }
+ }
+
+ test("division on negative-scale decimal falls back and returns correct results") {
+ // End-to-end proof for issue #5013 that the previously-panicking queries now complete.
+ // `checkSparkAnswerAndFallbackReason` rather than `checkSparkAnswer`: these five serdes do
+ // not mix in `CodegenDispatchFallback`, so `Unsupported` falls the whole projection back to
Review Comment:
All three updated. "Five serdes" is two. The `WideDecimalBinaryExpr` note described a native path this test never reaches, so it is gone rather than reworded. The references to "an earlier revision of this PR" and "this PR rewrites" now describe the code as it stands, naming the two width tests directly.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4307635672
##########
native/core/src/execution/planner.rs:
##########
@@ -1511,14 +1511,20 @@ impl PhysicalPlanner {
left.data_type(&input_schema),
right.data_type(&input_schema),
) {
+ // The width test is done in `i16` rather than `u8`: a negative scale makes
+ // `s as u8` wrap to a huge value and `p - s as u8` underflow, which panics in debug
+ // builds and silently selects the wrong branch in release (`overflow-checks = false`).
+ // Precision and scale are both bounded well inside `i16`, so this cannot overflow.
Review Comment:
Thanks for checking that and walking it back rather than leaving it. I had written "silently selects the wrong branch in release" as an inference and never verified it, so that one is on me.
Corrected in the doc comment on `decimal128_add_sub_width`, which both call sites now point at, in the note on `negScaleDecimalRejection` and in the PR description. They now say a debug build panicked and a release build was unaffected.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-5093502800
Thanks @andygrove and @comphead, that would be great if #4799 covers it!
I'd be glad to help review #4799.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-5152669870
Thanks @comphead. Good point on nested types, my guards match `case d: DecimalType if d.scale < 0`, so they don't match `ArrayType(DecimalType(10, -1))` or a struct field with negative scale.
On plan-level scanning or per-expression guards, I don't have a strong view either way.
@andygrove, do you have a preference?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4307635672
##########
native/core/src/execution/planner.rs:
##########
@@ -1511,14 +1511,20 @@ impl PhysicalPlanner {
left.data_type(&input_schema),
right.data_type(&input_schema),
) {
+ // The width test is done in `i16` rather than `u8`: a negative scale makes
+ // `s as u8` wrap to a huge value and `p - s as u8` underflow, which panics in debug
+ // builds and silently selects the wrong branch in release (`overflow-checks = false`).
+ // Precision and scale are both bounded well inside `i16`, so this cannot overflow.
Review Comment:
Thanks for checking that and walking it back rather than leaving it. I had written "silently selects the wrong branch in release" as an inference and never verified it, so that one is on me.
Corrected here, in `create_modulo_expr`, in the note on `negScaleDecimalRejection` and in the PR description. All four now say a debug build panicked and a release build was unaffected.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-5441590325
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled.
A panic in debug and a silent wrong answer in release is exactly the kind of thing worth closing off quickly, and doing it at the serde boundary is a reasonable conservative first move. The comment on `CometMultiply` explaining why it deliberately has no guard is the sort of thing that saves the next person a lot of time.
Four questions.
**Where does `Unsupported` actually route these casts?**
Since #4728, `Unsupported` routes through the JVM codegen dispatcher rather than straight to Spark row execution. The dispatcher builds Arrow vectors for its inputs and outputs. Does `CometBatchKernelCodegen` accept a `DecimalType` with negative scale, and can Arrow's `DecimalVector` represent one? If the dispatcher accepts it and then fails or produces garbage, this fix would move the problem rather than remove it.
If the dispatcher does handle it correctly, a test asserting that a negative-scale cast runs through the dispatcher and matches Spark would be worth having. If it does not, these need `withFallbackReason` plus something that keeps them out of the dispatcher too.
**The native kernel is still unsafe**
Guarding in Scala stops the Spark planner from reaching the kernel, but `10_i128.pow(scale as u32)` with a negative scale is still a live panic in the Rust code, reachable from any other caller: the Iceberg native scan, a UDF, a future serde. Should the kernel also return an error rather than compute `10^4294967295`? Even a `debug_assert!` plus an explicit `Err` would turn a silent wrap in release into something diagnosable. Is there an issue tracking the real fix, so this does not stay a permanent fallback?
**Only `BinaryArithmetic` is guarded**
`negScaleDecimalRejection` takes a `BinaryArithmetic`, so it covers `Add`, `Subtract`, `Divide`, `IntegralDivide`, and `Remainder`. What about the other places that scale-align decimals? `Round` and `BRound` compute a scale delta, `Sum` and `Average` rescale their buffers, `UnaryMinus` and `Abs` at least touch the type, and `CheckOverflow` / `DecimalRescaleCheckOverflow` do explicit rescaling. Did you audit those for the same `10^|scale|` pattern? If they are safe, saying so in the description would close the question. If they are not, they belong in this PR.
**Is the `Multiply` exemption pinned by the right test?**
The comment says multiply does not scale-align, so negative scale is safe. `WideDecimalBinaryExpr`'s multiply branch does compute a scale adjustment against the output precision and scale, so I would want the regression test to cover a negative *output* scale specifically, not just negative input scales. Does the test in `CometExpressionSuite` do that? If so, referencing it by name in the comment would make the exemption self-verifying.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4307690783
##########
spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegenInput.scala:
##########
@@ -245,6 +245,18 @@ private[codegen] object CometBatchKernelCodegenInput extends CometTypeShim {
}
}
+ /**
+ * True when a decimal column can be read through the unscaled-long fast path.
+ *
+ * `Decimal.createUnsafe` produces a long-backed `Decimal` whose `decimalVal` is null, and
+ * Spark's conversions then index `Decimal.POW_10(scale)`. That throws for a negative scale,
+ * which `spark.sql.legacy.allowNegativeScaleOfDecimal=true` allows, so a `try_cast` would
+ * silently return NULL and a plain cast would fail. The `BigDecimal`-backed reader has no such
+ * assumption, so negative scales take it regardless of precision.
+ */
+ private def canReadDecimalAsUnscaledLong(dt: DecimalType): Boolean =
+ dt.precision <= Decimal.MAX_LONG_DIGITS && dt.scale >= 0
Review Comment:
Fixed, and your query is now a test.
I confirmed it has teeth by reverting just the scale check and re-running: `ArrayIndexOutOfBoundsException: Index -1 out of bounds for length 19`, the same error you saw. `Decimal(20,-5)` is in the test too, to pin that the `BigDecimal` branch stays unaffected.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-6252015864
Thanks for the extra context on the 1.2.0 regression and on vanilla Spark's behavior across a shuffle. Agreed that returning the value is the right side to be on.
All six are addressed. Verified: `spotless:check` clean, `./mvnw -B test-compile -Pspark-3.5,delta -Pstrict-warnings -DskipTests` clean, `cargo clippy --all-targets --workspace -- -D warnings` clean, and nine suites green with 606 passing.
One thing worth mentioning: moving the width calculation out left `std::cmp::max` unused in `planner.rs`, which `make core` does not flag because it only builds the lib. Clippy caught it.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4125374680
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -452,13 +471,19 @@ object CometCast
case _ => unsupported(DataTypes.DoubleType, toType)
}
- private def canCastFromDecimal(toType: DataType): SupportLevel = toType match {
- case DataTypes.FloatType | DataTypes.DoubleType | DataTypes.ByteType | DataTypes.ShortType |
- DataTypes.IntegerType | DataTypes.LongType | DataTypes.BooleanType |
- DataTypes.TimestampType =>
- Compatible()
- case _ => Unsupported(Some(s"Cast from DecimalType to $toType is not supported"))
- }
+ private def canCastFromDecimal(fromType: DecimalType, toType: DataType): SupportLevel =
+ toType match {
+ // Negative-scale source either overflows or loses precision natively; see #5013.
+ case DataTypes.ByteType | DataTypes.ShortType | DataTypes.IntegerType | DataTypes.LongType |
+ DataTypes.FloatType | DataTypes.DoubleType | DataTypes.TimestampType
+ if fromType.scale < 0 =>
+ Unsupported(Some(negativeScaleDecimalCastReason))
Review Comment:
[P2] Could you make the dispatcher’s negative-scale decimal input reader safe before enabling this route? With `allowNegativeScaleOfDecimal=true`, a materialized `Decimal(10,-1)` column containing `100`, `-100`, and `0` now returns NULL for every `try_cast(v AS INT)`, instead of those integers. The dispatcher selects `Decimal.createUnsafe` for precision <= 18. Spark’s integer conversion then indexes `POW_10(-1)`, and TRY mode catches the resulting exception and silently returns NULL. Before this guard, TRY casts used the native Arrow path, which returns the correct values. The added tests collapse the string-to-decimal construction into the dispatched expression and therefore miss this input boundary. Use the BigDecimal-backed reader for negative scales, or ensure genuine Spark fallback until that reader is safe.
Evidence: Reproduced twice on this exact head with a debug native build and Spark 4.1.3/JDK 17. Enable `spark.sql.legacy.allowNegativeScaleOfDecimal=true` and Comet local-table scans, and exclude `ConvertToLocalRelation`. Run `Seq("100", "-100", "0").toDF("s").select(col("s").cast(DecimalType(10,-1)).as("v"), rand(123).as("r")).selectExpr("try_cast(v as int) as i", "r + r as r2")`. The nondeterministic projection preserves the decimal input boundary. Spark returns integer values 100, -100, and 0; Comet returns three NULLs. A second test excluding `CollapseProject` reproduces this without `rand`. The unchanged native TRY cast kernel separately returned `[Some(100), Some(-100), Some(0), None]`. Sources and logs are preserved under `/tmp/review5050-final-cqri1bx4/`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r3696036995
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -441,13 +437,18 @@ object CometCast
case _ => unsupported(DataTypes.DoubleType, toType)
}
- private def canCastFromDecimal(toType: DataType): SupportLevel = toType match {
- case DataTypes.FloatType | DataTypes.DoubleType | DataTypes.ByteType | DataTypes.ShortType |
- DataTypes.IntegerType | DataTypes.LongType | DataTypes.BooleanType |
- DataTypes.TimestampType =>
- Compatible()
- case _ => Unsupported(Some(s"Cast from DecimalType to $toType is not supported"))
- }
+ private def canCastFromDecimal(fromType: DecimalType, toType: DataType): SupportLevel =
+ toType match {
+ // Negative-scale source overflows natively; see #5013.
+ case DataTypes.ByteType | DataTypes.ShortType | DataTypes.IntegerType | DataTypes.LongType |
+ DataTypes.TimestampType if fromType.scale < 0 =>
Review Comment:
should we do early return here?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4307626322
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -511,12 +527,26 @@ object CometCast
private def canCastFromDecimal(fromType: DecimalType, toType: DataType): SupportLevel =
toType match {
- case DataTypes.ByteType | DataTypes.ShortType | DataTypes.IntegerType | DataTypes.LongType
- if fromType.scale < 0 =>
- Unsupported(Some(negativeScaleDecimalToIntegralReason))
- case DataTypes.FloatType | DataTypes.DoubleType | DataTypes.ByteType | DataTypes.ShortType |
- DataTypes.IntegerType | DataTypes.LongType | DataTypes.BooleanType |
- DataTypes.TimestampType =>
+ // The integral and timestamp kernels scale-align through a power of ten that overflows for
+ // a negative scale. Matching each target group once keeps the scale check next to the list
+ // it applies to. See issue #5013.
+ case DataTypes.ByteType | DataTypes.ShortType | DataTypes.IntegerType |
+ DataTypes.LongType =>
+ if (fromType.scale < 0) {
+ Unsupported(Some(negativeScaleDecimalToIntegralReason))
+ } else {
+ Compatible()
+ }
+ case DataTypes.TimestampType =>
+ if (fromType.scale < 0) {
+ Unsupported(Some(negativeScaleDecimalToTimestampReason))
+ } else {
+ Compatible()
+ }
+ // Float and double are not gated on scale: `decimal128_to_f64` / `decimal128_to_f32`
+ // round the exact decimal value once and handle a negative scale deliberately (#5684).
+ case DataTypes.FloatType | DataTypes.DoubleType | DataTypes.BooleanType =>
+ Compatible()
Compatible()
Review Comment:
Mine from the #6953 merge. Dropped.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-6314274163
Thanks @0lai0, all six look good. `cargo fmt --check` was failing on the new `decimal128_add_sub_width` re-export in `native/spark-expr/src/lib.rs`, so I pushed daad29586 with just that formatting fix, approved CI, and enabled auto-merge.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4303075483
##########
native/core/src/execution/planner.rs:
##########
@@ -1511,14 +1511,20 @@ impl PhysicalPlanner {
left.data_type(&input_schema),
right.data_type(&input_schema),
) {
+ // The width test is done in `i16` rather than `u8`: a negative scale makes
+ // `s as u8` wrap to a huge value and `p - s as u8` underflow, which panics in debug
+ // builds and silently selects the wrong branch in release (`overflow-checks = false`).
+ // Precision and scale are both bounded well inside `i16`, so this cannot overflow.
Review Comment:
I need to walk back something from my first review. The `u8` version of this test never picked the wrong branch in release builds. `max(s1, s2)` is taken on the `i8` values before the cast, and with overflow checks off the `u8` subtraction and addition wrap modulo 256 right back to the true value. I checked the wrapped expression against your `i16` one for every precision from 1 to 38 and every scale from -128 up to the precision, and they agree on all 31,843,449 combinations. So the bug was the debug-build panic. The `i16` rewrite is still the right fix. Could this comment, the one in `create_modulo_expr`, the note on `negScaleDecimalRejection` and the PR description say that debug builds panicked, rather than that release took the wrong branch?
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -511,12 +527,26 @@ object CometCast
private def canCastFromDecimal(fromType: DecimalType, toType: DataType): SupportLevel =
toType match {
- case DataTypes.ByteType | DataTypes.ShortType | DataTypes.IntegerType | DataTypes.LongType
- if fromType.scale < 0 =>
- Unsupported(Some(negativeScaleDecimalToIntegralReason))
- case DataTypes.FloatType | DataTypes.DoubleType | DataTypes.ByteType | DataTypes.ShortType |
- DataTypes.IntegerType | DataTypes.LongType | DataTypes.BooleanType |
- DataTypes.TimestampType =>
+ // The integral and timestamp kernels scale-align through a power of ten that overflows for
+ // a negative scale. Matching each target group once keeps the scale check next to the list
+ // it applies to. See issue #5013.
+ case DataTypes.ByteType | DataTypes.ShortType | DataTypes.IntegerType |
+ DataTypes.LongType =>
+ if (fromType.scale < 0) {
+ Unsupported(Some(negativeScaleDecimalToIntegralReason))
+ } else {
+ Compatible()
+ }
+ case DataTypes.TimestampType =>
+ if (fromType.scale < 0) {
+ Unsupported(Some(negativeScaleDecimalToTimestampReason))
+ } else {
+ Compatible()
+ }
+ // Float and double are not gated on scale: `decimal128_to_f64` / `decimal128_to_f32`
+ // round the exact decimal value once and handle a negative scale deliberately (#5684).
+ case DataTypes.FloatType | DataTypes.DoubleType | DataTypes.BooleanType =>
+ Compatible()
Compatible()
Review Comment:
Looks like the merge in `7c940c626` left a second `Compatible()` here. It's harmless since the block returns the last expression, but could you drop one?
##########
spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegenInput.scala:
##########
@@ -245,6 +245,18 @@ private[codegen] object CometBatchKernelCodegenInput extends CometTypeShim {
}
}
+ /**
+ * True when a decimal column can be read through the unscaled-long fast path.
+ *
+ * `Decimal.createUnsafe` produces a long-backed `Decimal` whose `decimalVal` is null, and
+ * Spark's conversions then index `Decimal.POW_10(scale)`. That throws for a negative scale,
+ * which `spark.sql.legacy.allowNegativeScaleOfDecimal=true` allows, so a `try_cast` would
+ * silently return NULL and a plain cast would fail. The `BigDecimal`-backed reader has no such
+ * assumption, so negative scales take it regardless of precision.
+ */
+ private def canReadDecimalAsUnscaledLong(dt: DecimalType): Boolean =
+ dt.precision <= Decimal.MAX_LONG_DIGITS && dt.scale >= 0
Review Comment:
`CometVector.getDecimal` has the same fast path, at `CometVector.java:91`, and Spark reads every Comet batch through it after `CometColumnarToRow`. The new division fallback sends exactly that shape to Spark. With your test confs (legacy flag on, local table scan on, `ConvertToLocalRelation` excluded) and `CollapseProject` left alone, this keeps two projections because `v` is used twice:
```scala
Seq("100", "-100", "0").toDF("s")
.select(col("s").cast(DecimalType(10, -1)).as("v"))
.selectExpr("v / cast(3 as decimal(10, 0)) as q", "cast(v as int) as i")
```
Spark returns `[33.3333333333,100]`, `[-33.3333333333,-100]` and `[0E-10,0]`. Comet runs the inner projection natively, falls back for the outer one, and fails with `ArrayIndexOutOfBoundsException: Index -1 out of bounds for length 19` when Spark's `cast(v as int)` indexes `POW_10(-1)`. `Decimal(20, -5)` is fine since it takes the `BigDecimal` branch. On `main` the division runs natively and reaches the `10.pow(~4.29e9)` in `decimal_div`, so this isn't a regression, but it is the same root cause you fixed here. Adding `&& scale >= 0` to that condition fixed it for me, and the negative-scale tests in both suites still passed. Could you fold that in, with this query as a test?
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1486,6 +1487,111 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
+ test("division on negative-scale decimal reports unsupported via getSupportLevel") {
+ // Issue #5013: `spark_decimal_div` computes its scale-alignment exponents in `u32`, so a
+ // negative scale wraps to roughly 4.29e9 and the BigInt branch attempts an unbounded
+ // allocation. Only division is affected. The other arithmetic operators scale-align by
+ // `10^(max_scale - s)`, which is never negative, so they stay native and are pinned by
+ // "safe ops on negative-scale decimal run natively" below. Constructing the expressions
+ // requires allowNegativeScaleOfDecimal so DecimalType(_, s<0) passes Spark's own check.
+ withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true") {
+ Seq(DecimalType(10, -1), DecimalType(20, -5)).foreach { negScale =>
+ val posDec = DecimalType(10, 0)
+ val negAttr = AttributeReference("n", negScale)()
+ val posAttr = AttributeReference("p", posDec)()
+ val expected = Unsupported(Some(CometDivide.negScaleDecimalArithmeticReason))
+
+ assert(CometDivide.getSupportLevel(Divide(negAttr, posAttr)) == expected)
+ assert(CometIntegralDivide.getSupportLevel(IntegralDivide(negAttr, posAttr)) == expected)
+ // Guard also fires when negative scale is only on the right operand.
+ assert(CometDivide.getSupportLevel(Divide(posAttr, negAttr)) == expected)
+
+ // The unguarded operators must not report unsupported for the same operands.
+ assert(CometAdd.getSupportLevel(Add(negAttr, posAttr)) == Compatible())
+ assert(CometSubtract.getSupportLevel(Subtract(negAttr, posAttr)) == Compatible())
+ assert(CometRemainder.getSupportLevel(Remainder(negAttr, posAttr)) == Compatible())
+ assert(CometMultiply.getSupportLevel(Multiply(negAttr, posAttr)) == Compatible())
+ }
+ }
+ }
+
+ test("division on negative-scale decimal falls back and returns correct results") {
+ // End-to-end proof for issue #5013 that the previously-panicking queries now complete.
+ // `checkSparkAnswerAndFallbackReason` rather than `checkSparkAnswer`: these five serdes do
+ // not mix in `CodegenDispatchFallback`, so `Unsupported` falls the whole projection back to
+ // Spark, and asserting the reason is what proves the guard fired. A plain answer comparison
+ // would pass whether or not it did.
+ // ConvertToLocalRelation is excluded so the arithmetic actually runs on the plan rather
+ // than being folded at plan time (#4789). `DecimalType(_, s<0)` must be constructed under
+ // allowNegativeScaleOfDecimal=true because the case class initializer reads the flag, so
+ // wrap everything (including the type value) in one block.
+ withSQLConf(
+ "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true",
+ CometConf.COMET_EXEC_LOCAL_TABLE_SCAN_ENABLED.key -> "true",
+ SQLConf.OPTIMIZER_EXCLUDED_RULES.key ->
+ "org.apache.spark.sql.catalyst.optimizer.ConvertToLocalRelation") {
+ // Cover a narrow and a wide negative-scale type: at precision 20 the combined operand
+ // width crosses DECIMAL128_MAX_PRECISION, which routes the operation to
+ // `WideDecimalBinaryExpr` / `decimal_div`'s BigInt path rather than the narrow arrow
+ // kernels. `unit` is the smallest magnitude representable at that scale.
+ val negScaleCases = Seq((DecimalType(10, -1), 10), (DecimalType(20, -5), 100000))
+ negScaleCases.foreach { case (negScaleType, unit) =>
+ // Build the neg-scale column via string -> Decimal(neg), a known-safe cast, so the
+ // source construction does not depend on any of the guards under test.
+ val strs = Seq(10 * unit, 20 * unit, 30 * unit).map(_.toString)
+ val v = strs.toDF("s").select(col("s").cast(negScaleType).as("v"))
+ Seq("v / cast(3 as decimal(10, 0))", "v div cast(3 as decimal(10, 0))").foreach { e =>
+ checkSparkAnswerAndFallbackReason(
+ v.selectExpr(e),
+ CometDivide.negScaleDecimalArithmeticReason)
+ }
+ }
+ }
+ }
+
+ test("safe ops on negative-scale decimal run natively (regression pin)") {
+ // Multiply and UnaryMinus on negative-scale decimals don't scale-align an integer and
+ // ran natively at the time #5013 was fixed. The arithmetic guard intentionally does
+ // not cover them. If a future change ever adds scale-alignment to Multiply or a
+ // decimal codepath is added to UnaryMinus that does the multiply-by-10^n trick,
+ // these assertions will start returning a Comet fallback plan and this test will
+ // catch it before it reaches production as a silent-wrap or debug panic.
+ withSQLConf(
+ "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true",
+ CometConf.COMET_EXEC_LOCAL_TABLE_SCAN_ENABLED.key -> "true",
+ SQLConf.OPTIMIZER_EXCLUDED_RULES.key ->
+ "org.apache.spark.sql.catalyst.optimizer.ConvertToLocalRelation") {
+ // The wide case matters most for `v * v`: Decimal(20,-5) * Decimal(20,-5) has
+ // p1 + p2 >= DECIMAL128_MAX_PRECISION, so it takes `WideDecimalBinaryExpr`'s multiply
+ // branch (which does compute a scale adjustment against the output precision/scale)
+ // and its result type is Decimal(38,-10) -- a negative *output* scale, not just
+ // negative inputs.
+ val negScaleCases = Seq((DecimalType(10, -1), 10), (DecimalType(20, -5), 100000))
+ negScaleCases.foreach { case (negScaleType, unit) =>
+ val strs = Seq(10 * unit, 20 * unit, 30 * unit).map(_.toString)
+ val v = strs.toDF("s").select(col("s").cast(negScaleType).as("v"))
+ // Same-scale Multiply - no alignment needed, must stay native.
+ checkSparkAnswerAndOperator(v.selectExpr("v * v"))
+ checkSparkAnswerAndOperator(v.selectExpr("v * cast(2 as int)"))
+ // UnaryMinus on Decimal(neg) - no alignment, must stay native.
+ checkSparkAnswerAndOperator(v.selectExpr("-v"))
+ // Add, Subtract and Remainder scale-align by `10^(max_scale - s)`, which is never
+ // negative. They were guarded in an earlier revision of this PR because the `u8`
+ // underflow in the two width tests made them fail. Those are rewritten in `i16` here,
+ // so they run natively again. If either rewrite is reverted these assertions fail.
Review Comment:
CI builds the native library with `--profile ci`, which inherits `overflow-checks = false` from release. So if either width rewrite were reverted, these assertions would still pass in CI. The `cargo nextest` job runs with overflow checks on, though. Would you be up for adding a negative-scale case next to `test_modulo_basic_decimal` in `modulo_expr.rs` and next to `test_decimal_divide_by_zero_returns_spark_error` in `div.rs`? That would pin the modulo rewrite and also cover the new `decimal_div` rejection, which has no test yet. The planner copy is harder to reach from a unit test. Moving the width calculation into one small function that both sites call would make it testable and leave one copy of the expression instead of two.
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1486,6 +1487,111 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
+ test("division on negative-scale decimal reports unsupported via getSupportLevel") {
+ // Issue #5013: `spark_decimal_div` computes its scale-alignment exponents in `u32`, so a
+ // negative scale wraps to roughly 4.29e9 and the BigInt branch attempts an unbounded
+ // allocation. Only division is affected. The other arithmetic operators scale-align by
+ // `10^(max_scale - s)`, which is never negative, so they stay native and are pinned by
+ // "safe ops on negative-scale decimal run natively" below. Constructing the expressions
+ // requires allowNegativeScaleOfDecimal so DecimalType(_, s<0) passes Spark's own check.
+ withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true") {
+ Seq(DecimalType(10, -1), DecimalType(20, -5)).foreach { negScale =>
+ val posDec = DecimalType(10, 0)
+ val negAttr = AttributeReference("n", negScale)()
+ val posAttr = AttributeReference("p", posDec)()
+ val expected = Unsupported(Some(CometDivide.negScaleDecimalArithmeticReason))
+
+ assert(CometDivide.getSupportLevel(Divide(negAttr, posAttr)) == expected)
+ assert(CometIntegralDivide.getSupportLevel(IntegralDivide(negAttr, posAttr)) == expected)
+ // Guard also fires when negative scale is only on the right operand.
+ assert(CometDivide.getSupportLevel(Divide(posAttr, negAttr)) == expected)
+
+ // The unguarded operators must not report unsupported for the same operands.
+ assert(CometAdd.getSupportLevel(Add(negAttr, posAttr)) == Compatible())
+ assert(CometSubtract.getSupportLevel(Subtract(negAttr, posAttr)) == Compatible())
+ assert(CometRemainder.getSupportLevel(Remainder(negAttr, posAttr)) == Compatible())
+ assert(CometMultiply.getSupportLevel(Multiply(negAttr, posAttr)) == Compatible())
+ }
+ }
+ }
+
+ test("division on negative-scale decimal falls back and returns correct results") {
+ // End-to-end proof for issue #5013 that the previously-panicking queries now complete.
+ // `checkSparkAnswerAndFallbackReason` rather than `checkSparkAnswer`: these five serdes do
+ // not mix in `CodegenDispatchFallback`, so `Unsupported` falls the whole projection back to
Review Comment:
A few comments in these tests still describe earlier revisions. "These five serdes" is two now. The note at line 1533 about precision 20 routing to `WideDecimalBinaryExpr` or the BigInt path describes a native path this test never reaches, since it asserts the Spark fallback, and division doesn't use `WideDecimalBinaryExpr`. Line 1579 and the comment in `arithmetic.scala` at line 100 refer to "an earlier revision of this PR" and "this PR rewrites", which won't mean much after merge. Could you bring them in line with the code as it is?
##########
docs/source/user-guide/latest/compatibility/expressions/_category_template/cast.md:
##########
@@ -165,6 +165,22 @@ Spark's behavior of using Java `BigDecimal.toString()` semantics, which produces
notation (e.g. a value of 12300 stored as `Decimal(7,-2)` with unscaled value 123 is rendered
as `"1.23E+4"`).
+## Decimal with Negative Scale to Integer and Timestamp
+
+Casting a `DecimalType` with a negative scale to `ByteType`, `ShortType`, `IntegerType`,
+`LongType` or `TimestampType` has no native implementation, and neither does casting any of the
+integer types to a negative-scale `DecimalType`. Comet reports these as unsupported, which
+routes them through the JVM codegen dispatcher (Spark's own generated code) inside the Comet
+pipeline, so the results match Spark exactly.
+
+The native kernels align scale by computing `10^|scale|` in fixed-point arithmetic, which
+overflows when the scale is negative. See
+[#5013](https://github.com/apache/datafusion-comet/issues/5013).
+
+All other cast directions to and from negative-scale decimals run natively: `FloatType` and
+`DoubleType`, `StringType` in both directions (subject to the config gate described above),
+`BooleanType`, and widening to another `DecimalType`.
Review Comment:
`BooleanType` only runs natively in one direction. A negative-scale decimal to `BooleanType` is native, but `canCastFromBoolean` sends every `BooleanType` to `DecimalType` cast through the dispatcher, whatever the scale. Could this say casts from a negative-scale decimal to `BooleanType`?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4288703046
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -441,13 +437,18 @@ object CometCast
case _ => unsupported(DataTypes.DoubleType, toType)
}
- private def canCastFromDecimal(toType: DataType): SupportLevel = toType match {
- case DataTypes.FloatType | DataTypes.DoubleType | DataTypes.ByteType | DataTypes.ShortType |
- DataTypes.IntegerType | DataTypes.LongType | DataTypes.BooleanType |
- DataTypes.TimestampType =>
- Compatible()
- case _ => Unsupported(Some(s"Cast from DecimalType to $toType is not supported"))
- }
+ private def canCastFromDecimal(fromType: DecimalType, toType: DataType): SupportLevel =
+ toType match {
+ // Negative-scale source overflows natively; see #5013.
+ case DataTypes.ByteType | DataTypes.ShortType | DataTypes.IntegerType | DataTypes.LongType |
+ DataTypes.TimestampType if fromType.scale < 0 =>
Review Comment:
Good call, thanks. Restructured, though not as an early return.
An early return would not have helped, because the guard only covers a subset of the target types, so the `Byte | Short | Integer | Long | Timestamp` list had to appear twice either way. The duplication was the problem, not the branch position.
It now matches that list once and does the `fromType.scale < 0` check inside that case, with the comment about why float and double are deliberately excluded sitting on the case that excludes them. Semantics unchanged, `CometNativeCastSuite` and `CometExpressionSuite` pass.
##########
spark/src/main/scala/org/apache/comet/serde/arithmetic.scala:
##########
@@ -91,6 +90,25 @@ trait MathBase {
Unsupported(Some(s"Unsupported datatype $dt"))
}
+ // Native decimal Add/Subtract/Divide/IntegralDivide/Remainder scale-aligns operands by
+ // multiplying by 10^|delta|, which overflows (subtract-with-overflow panic in debug, silent
+ // wrap in release) whenever any operand or the result has negative scale. See issue #5013.
+ private[comet] val negScaleDecimalArithmeticReason: String =
+ "Arithmetic on negative-scale decimal is not supported natively"
+
+ private[comet] def negScaleDecimalRejection(expr: BinaryArithmetic): Option[Unsupported] = {
Review Comment:
You were right @comphead, and most of this file's changes have since come out. Sorry for the delay.
I had attributed the panic to the operators scale-aligning by `10^|delta|`, which was wrong. Add and subtract align by `10^(max_scale - s1)`, never negative. The real cause was two width tests computing `max(s1, s2) as u8 + max(p1 - s1 as u8, p2 - s2 as u8)`, where a negative scale wraps `s as u8` to 255 and underflows the subtraction. Both are now rewritten in `i16`.
So the guards on `CometAdd`, `CometSubtract` and `CometRemainder` are gone, along with their `getUnsupportedReasons` overrides. I verified each runs natively and matches Spark over `Decimal(10,-1)`, `Decimal(20,-5)` and `Decimal(38,-5)`, and they are pinned in `safe ops on negative-scale decimal run natively`.
Only `CometDivide` and `CometIntegralDivide` still need it. `spark_decimal_div` computes its exponents in `u32`, so `s1 = -5` becomes ~4.29e9 and the BigInt branch attempts `10.pow(~4.29e9)`, an unbounded allocation that hangs rather than fails. `decimal_div` now rejects a negative scale up front, but there is still no usable native result. The comment carries that reason now.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-5558329269
Thanks @andygrove and @comphead.
## Where `Unsupported` routes these casts
`CometCast` gained `CodegenDispatchFallback` after this PR branched, so these casts route through the JVM codegen dispatcher rather than falling back to Spark, my three `checkSparkAnswerAndFallbackReason` assertions failed after the rebase for exactly that reason (`Accelerated expressions: 0 native, 1 codegen dispatch`).
It is not moving the problem, though. The dispatcher delegates to Spark's own `doGenCode` (`Decimal`/`BigDecimal`), and Arrow carries the unscaled value with the scale as metadata, `writeBigDecimalToArrowBuf` just calls `value.unscaledValue()`. No power-of-ten arithmetic on that path. Confirmed on unmodified `main` via `Boolean -> Decimal`, which is already dispatcher-routed: passes for both `Decimal(10,-1)` and `Decimal(20,-5)` with no panic. `main`'s own `round on negative-scale decimal` test relies on the same mechanism.
Tests now assert `assertCodegenRan { checkSparkAnswerAndOperator(...) }`, and I have corrected the PR description. The arithmetic half is a genuine Spark fallback, those five serdes don't mix the trait in.
## Parameterising over `Decimal(20,-5)` caught a real bug
```
!== Spark Answer == == Comet Answer ==
![1000000.0] [999999.9999999999]
```
`Decimal(neg) -> Double` has no Comet arm in `cast.rs`, so arrow's kernel computes `unscaled / 10f64.powi(scale)`. For scale `-5` the divisor `1e-5` is not exactly representable; the Comet values reproduce bit-for-bit as `10/1e-5` etc. Spark's `BigDecimal.doubleValue()` is exact. Positive scales are fine (`10^s` exact for `s <= 22`).
So my claim that all other directions were "unaffected" was wrong, they avoid the *panic*, but float/double was silently wrong. `canCastFromDecimal` now guards those two as well.
The `v * v` pin also covers `Decimal(20,-5) * Decimal(20,-5)` now, which takes `WideDecimalBinaryExpr`'s multiply branch and yields `Decimal(38,-10)` — a negative *output* scale.
## Other items
- **`Boolean -> Decimal(neg)`** — already fixed on `main` independently; nothing needed after the rebase.
- **`planner.rs` underflow** — rewritten in `i16`.
- **Other rescaling sites** — all safe as written, no changes: `Round`/`Ceil`/`Floor` already guarded; `sum_decimal.rs` has no `pow()` and never rescales; `avg_decimal.rs`'s exponent is `(s+4) - s = 4` regardless of sign; `decimal_rescale_check.rs` uses signed `i8` + `unsigned_abs()` and rejects `abs_delta > 38`.
- **Docs** — new `cast.md` section (the generated matrix only samples `Decimal(10,2)`), and the five arithmetic serdes now override `getUnsupportedReasons()` for `math.md`.
## Consolidating the negative-scale check
Agreed, including @comphead's nested-type point (`ArrayType(DecimalType(10,-1))` isn't matched today) and your `exprToProtoInternal` + `SupportLevel.containsType` suggestion. It would touch the pre-existing `Ceil`/`Floor`/`Round` guards too, so I'd rather file it as a follow-up than grow this PR, happy to pick it up if that works.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 closed pull request #5050: fix: fall back to Spark for negative-scale decimal casts and arithmetic
URL: https://github.com/apache/datafusion-comet/pull/5050
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for negative-scale decimal casts and arithmetic [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-6234083283
Heads up @andygrove @sunchao @comphead, the merge in `7c940c626` changed this PR enough that it is worth re-reading before it goes in, since the approval predates it.
#6953 landed a guard for `Decimal(neg) -> integral` on `main`, with `negativeScaleDecimalToIntegralReason` and a test covering all four integral types across LEGACY, ANSI and TRY. That overlapped with half of what this PR was doing, so I took `main`'s version rather than mine and kept only the directions it does not cover:
| direction | now owned by |
| --- | --- |
| `Decimal(neg) -> integral` | `main`, via #6953 |
| `Decimal(neg) -> Timestamp` | this PR, `negativeScaleDecimalToTimestampReason` |
| `integral -> Decimal(neg)` | this PR, `negativeScaleDecimalFromIntegralReason` |
`canCastFromDecimal` matches each target group once and does the scale check inside that case, which keeps the de-duplication @comphead asked for while covering both the integral group from `main` and the timestamp group from here. `main`'s own test passes unchanged against that shape, so the two are equivalent.
On the test side I kept `main`'s integral test as is, since it is more thorough than what I had, and narrowed mine to the two remaining directions plus the dispatcher routing assertions that `main`'s test does not make.
One consequence worth flagging: `CometCast` now carries four negative-scale reason strings, counting the pre-existing to-string one. That is squarely what #7021 is about, and this merge makes it more worth doing rather than less.
Also settled by the merge: `main`'s test asserts `Unsupported` for TRY as well, which matches @andygrove's call to leave the `evalMode` scoping alone, so that question is closed.
Verified on the merge result: `cargo clippy --all-targets --workspace -- -D warnings` clean, `./mvnw spotless:check` clean, `./mvnw -B test-compile -Pspark-3.5,delta -Pstrict-warnings -DskipTests` clean (that is what the earlier red run caught, an `implicit numeric widening` in a test I added), and nine suites green with 605 passing and 0 failures.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org