fix: fall back to Spark for negative-scale decimal casts and arithmetic

[0lai0]

Which issue does this PR close?

Closes #5013

Rationale for this change

With spark.sql.legacy.allowNegativeScaleOfDecimal=true, Spark allows DecimalType(p, s) with s < 0. Comet's native kernels are not written for that case, in two distinct ways:

• Overflow. Casts between integer/timestamp types and negative-scale decimals, and decimal Add/Subtract/Divide/IntegralDivide/Remainder, scale-align by computing 10^|scale| (e.g. 10_i128.pow(scale as u32)). A negative scale makes as u32 produce a huge exponent, so the power computation panics in debug builds and silently wraps in release (overflow-checks = false).

• Precision loss. Decimal(neg) -> Float/Double has no Comet arm in cast.rs and falls through to arrow's kernel, which divides by 10^scale. For a negative scale that divisor is not exactly representable, so the result silently diverges from Spark's exact BigDecimal.doubleValue() — a Decimal(20,-5) holding 1000000 comes back as 999999.9999999999.

Reproduced on main in a debug build:

Comet native panic: attempt to multiply with overflow
THREW: org.apache.comet.CometNativeException: attempt to multiply with overflow

allowNegativeScaleOfDecimal is a rarely-used legacy flag (default false) and the native kernels don't implement negative-scale-safe rescaling, so this PR detects these cases at planning time and takes them off the native path.

What changes are included in this PR?

Casts — CometCast.scala. Reports Unsupported for int -> Decimal(neg) (via canCastFromByte/Short/Int/Long) and for Decimal(neg) -> int/Float/Double/Timestamp (via canCastFromDecimal, which now takes the source DecimalType so it can check fromType.scale).

Note that CometCast mixes in CodegenDispatchFallback, so Unsupported here does not fall the projection back to Spark — it routes through the JVM codegen dispatcher and stays in the Comet pipeline. That path is safe for negative scale: it runs Spark's own doGenCode over Decimal/BigDecimal, and Arrow carries the unscaled value with the scale as metadata, so neither the overflow nor the inexact-divisor problem is reachable. The tests assert both that the dispatcher ran and that results match Spark.

All other directions to and from negative-scale decimals still run natively — String both ways, Boolean, and widening to another DecimalType — and are pinned by a regression test.

Arithmetic — arithmetic.scala. negScaleDecimalRejection(expr: BinaryArithmetic) in MathBase returns Unsupported when either operand or the result is a negative-scale decimal, wired into CometAdd, CometSubtract, CometDivide, CometIntegralDivide and CometRemainder. These five do not mix in CodegenDispatchFallback, so here Unsupported is a genuine Spark fallback. Each also overrides getUnsupportedReasons() so the generated math.md carries the caveat.

CometMultiply and UnaryMinus are intentionally left unguarded — neither scale-aligns.

Native — planner.rs. The decimal width test computed max(s1, s2) as u8 + max(p1 - s1 as u8, p2 - s2 as u8); with a negative scale s as u8 wraps to 255 and the subtraction underflows. Rewritten in i16. The Scala guards mean this is no longer reachable from the Spark planner, but it removes the landmine for other callers.

Docs. New _category_template/cast.md section covering these pairs, since the generated matrix only samples createDecimalType(10, 2).

How are these changes tested?

New and updated tests in CometNativeCastSuite and CometExpressionSuite, parameterised over both DecimalType(10, -1) and DecimalType(20, -5). The wider type matters: combined operands cross DECIMAL128_MAX_PRECISION, selecting WideDecimalBinaryExpr and decimal_div's BigInt path rather than the narrow arrow kernels. It also pins CometMultiply against a negative output scale — Decimal(20,-5) * Decimal(20,-5) yields Decimal(38,-10).

Verified locally against a debug native build, where the panic surfaces:

./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometExpressionSuite,\
org.apache.comet.CometMathExpressionSuite,org.apache.comet.CometNativeCastSuite"

Tests: succeeded 329, failed 0, canceled 0, ignored 12, pending 0

Note: the Timestamp and Float/Double cases are not in the linked issue's reproducer. Both were found while auditing the same code paths, share the root cause (no negative-scale-safe rescaling natively), and are included here. The float/double divergence was caught by parameterising over DecimalType(20, -5), per review feedback.

[andygrove]

This may overlap with #4799

[0lai0]

Thanks @andygrove and @comphead, sorry for missing #4799! That would be great if it covers it.
I'd be glad to help review #4799.

[andygrove]

This is a small targeted PR that seems ready for review and #4799 is still in flux. Let's reopen this one.

[andygrove]

Thanks for this @0lai0. The analysis in the description is careful and mostly holds up. I traced every pow(scale as u32) site in native/ against your guards, and the ones you deliberately left native really are safe:

• float/double → Decimal(neg): cast_floating_point_to_decimal128 uses 10_f64.powi(scale as i32) (numeric.rs:901)
• string ↔ Decimal(neg): sign-checked i32 branches (string.rs:603-620)
• Decimal(neg) → Boolean: spark_cast_decimal_to_boolean has no scale math (numeric.rs:860)
• Multiply: the wide path's scale_diff branches are sign-checked (wide_decimal_binary_expr.rs:256-275), and the narrow path short-circuits past the panicking subtraction in the planner guard, so leaving it unguarded is correct
• Avg: target_scale - sum_scale is (s+4) - s = 4 regardless of sign, so avg_decimal.rs:382 is fine
• Ceil/Floor/Round were already guarded

Also agree the Scala suites are the right home here rather than CometSqlFileTestSuite, since negative scale cannot be written in SQL.

A few things to address.

Rebase needed

This is showing as conflicting with main and no CI has run on the branch. CometCast has changed since 27 July, it now mixes in CometTypeShim and CodegenDispatchFallback, and getSupportLevel gained a literal-child short-circuit. Could you rebase so we get a green CI signal? I would like to see the new tests pass on a debug build before merging, since debug is where the panic surfaces.

Boolean → Decimal(negative scale) looks like the same bug, unguarded

CometCast.scala:347 (canCastFromBoolean) returns Compatible() for _: DecimalType with no scale check, and this PR does not touch it. cast.rs:441 sends (Boolean, Decimal128(p, s)) to cast_boolean_to_decimal, which does 10_i128.pow(scale as u32) at boolean.rs:37. That is the identical pattern you are guarding everywhere else.

Does col("b").cast(DecimalType(10, -1)) still panic with attempt to multiply with overflow? If so it needs the same case d: DecimalType if d.scale < 0 guard, plus a test alongside the integer ones.

Consolidating the negative-scale check

After this PR there are five places testing d.scale < 0 with four different message strings: the two new constants here, negativeScaleDecimalToStringReason, and CometCeil/CometFloor/CometRound each carrying their own "Decimal type has negative scale".

Would it be worth pulling out a single shared predicate, something like CometCast.isNegativeScaleDecimal(dt) with one reason string? The Boolean gap above is exactly the kind of thing that is easy to miss when the check is scattered, and this family of guards is likely to keep growing.

The native underflow stays armed

The attempt to subtract with overflow in #5013 comes from the match guard at native/core/src/execution/planner.rs:918:

max(s1, s2) as u8 + max(p1 - s1 as u8, p2 - s2 as u8) >= DECIMAL128_MAX_PRECISION

With s1 = -1, s1 as u8 is 255 and p1 - 255 underflows the u8. In release builds overflow-checks = false means it wraps to p1 + 1 and we quietly take a different branch instead of panicking.

Your Scala guards mean we should not reach this anymore, but could we also do that arithmetic in i16? It is a small change and it turns a landmine into an ordinary error for any future path that reaches native with a negative scale. If you would rather keep this PR tight, could you file a follow-up issue and link it here?

Test coverage is only DecimalType(10, -1)

CometCastSuite.scala:797 and CometExpressionSuite.scala:1054 both use DecimalType(10, -1), but #5013 also reproduced with DecimalType(20, -5), and the scale magnitude selects a different native branch. create_binary_expr_with_options routes wide operands to WideDecimalBinaryExpr rather than arrow's BinaryExpr, and decimal_div picks its BigInt path over the i128 one.

Could you parameterise over both DecimalType(10, -1) and DecimalType(20, -5)? It matters most for the v * v pin in "safe ops on negative-scale decimal run natively", since right now that only proves the narrow arrow path is safe.

Documentation

Two gaps worth closing.

The generated cast matrix only samples createDecimalType(10, 2) from CometCast.supportedTypes, so none of the new Unsupported pairs appear in the table and users have no way to learn about them. _category_template/cast.md:153 already has a hand-maintained "Decimal with Negative Scale to String" section, so a companion section covering integer ↔ Decimal(neg) and Decimal(neg) → Timestamp would fit naturally there.

On the arithmetic side, Add, Subtract, Divide, IntegralDivide and Remainder are all in QueryPlanSerde.mathExpressions:111, which means they get a math.md entry built from getUnsupportedReasons(). Could the five serdes override that to return negScaleDecimalArithmeticReason? Otherwise the compat page shows them as fully supported with no caveat.

[comphead]

should we do early return here?

[comphead]

do we really need changes in this file?

[comphead]

@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
      }
    }
  }

[0lai0]

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?

[andygrove]

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.

[andygrove]

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.

[0lai0]

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.

[andygrove]

Note on this review: the prose here was generated by an LLM (Claude Code) at my request while I worked through a review backlog. Unlike my previous LLM-assisted comment on this PR, the claims below were checked against a local debug build rather than inferred from reading, and I've noted where a check was attempted and left unresolved. Still, please treat it as suggestions to evaluate rather than as authoritative feedback, and push back on anything that's wrong or already handled.

Thanks for picking this back up. The rebase is clean, the assertCodegenRan assertions are exactly the right way to pin the dispatcher routing, and the audit you posted of the other rescaling sites holds up. I checked -v, abs(v), sum(v), avg(v), v * v and v * decimal(10,0) against a debug build on both Decimal(10,-1) and Decimal(20,-5) and they all run natively and match Spark, so the exemptions you argued for are right.

First, a process note. CI has never run on this branch. All six workflow runs on 036c2e0 are sitting at action_required waiting on fork-workflow approval, which is on me rather than you. I'll approve them. In the meantime I built the merge locally and ran CometNativeCastSuite and CometExpressionSuite on the default profile against a debug native lib, and got 331 passing with 0 failures, so the PR as written is green.

The substance of what I found is that two of the guards are load-bearing, two are not, and there's a third copy of the underflow you're fixing that the PR doesn't touch. I isolated this by checking out the merge, keeping your planner.rs rewrite, reverting the Scala guards to main, and running each case against a debug build over a value set covering zero, negatives, nulls and max-magnitude values.

The float and double guard looks obsolete

I think the FloatType / DoubleType half of the canCastFromDecimal guard is no longer needed. #5684 landed on main on 2026-09-05, one revision before your merge commit, and it replaced arrow's (unscaled as f64) / 10^scale fall-through with decimal128_to_f64 and decimal128_to_f32 (native/spark-expr/src/conversion_funcs/numeric.rs:910 and :934). Those handle negative scale deliberately. The -22..=-1 arm multiplies by an exact F64_EXACT_POW10 entry, and anything outside that range goes through parse_exact_decimal, which formats the value as <unscaled>e<-scale> and hands it to Rust's correctly rounded float parser.

With that guard reverted, Decimal(10,-1) and Decimal(20,-5) both cast to double and to float fully natively and match Spark on every value I threw at them. The 999999.9999999999 you measured was real, it was just measured against 36d748f2e, and the merge one commit later fixed it underneath you.

Could you drop those two types from the guard and move them into the "safe casts run natively" pin instead? The cast.md section and the PR description need the same edit, since both currently document a divergence that no longer exists.

The Add and Subtract guards look unnecessary too

The comment on negScaleDecimalRejection says these scale-align by multiplying by 10^|delta|, but I can't find that in the Rust for add or subtract. The wide path computes i256_pow10((max_scale - s1) as u32) at wide_decimal_binary_expr.rs:221, and max_scale >= s1 by construction, so that exponent is never negative. The subsequent rescale is already i16 with sign checks. The narrow path just goes to arrow. The panic you attributed to them really does look like the p1 - s1 as u8 underflow at planner.rs, which this PR already repairs.

Empirically, with your i16 rewrite in and the CometAdd / CometSubtract guards out, v + v, v - v, v + decimal(10,0), v - decimal(10,0) and decimal(10,0) - v all run natively and match Spark. That includes Decimal(38,-5) via the DataFrame API, which is wide enough to take the WideDecimalBinaryExpr branch rather than the narrow arrow one.

Would you consider dropping those two guards along with their getUnsupportedReasons overrides, and pinning them as native regressions instead? These five serdes don't mix in CodegenDispatchFallback, so Unsupported here costs a whole-projection fallback to Spark, and it would be a shame to pay that for a case your own planner.rs fix already handles. If you'd rather keep them for conservatism I don't object, but then the comment should carry the real reason rather than the 10^|delta| one.

There is a second copy of the same underflow

create_modulo_expr at native/spark-expr/src/math_funcs/modulo_expr.rs:161 has the identical max(s1, s2) as u8 + max(p1 - s1 as u8, p2 - s2 as u8) expression that you're rewriting in planner.rs, and it's untouched here. That's why Remainder is the one arithmetic operator that genuinely still needs its guard. I confirmed v % v and v % decimal(10,0) fail with your planner.rs fix in place and the Scala guard out.

Could you apply the same i16 rewrite there? The "turns a landmine into an ordinary error for any future caller" argument you make for planner.rs applies word for word to this line, and leaving one of the two fixed is a strange place to end up. It's also worth re-checking afterwards whether CometRemainder still needs its guard at all, the way it turns out add and subtract don't. I started that experiment and the machine ran out of disk mid-rebuild, so I can't tell you the answer.

Divide is the nastiest of the set and worth describing accurately

At native/spark-expr/src/math_funcs/div.rs:100-101:

let l_exp = ((s2 + s3 + 1) as u32).saturating_sub(s1 as u32);
let r_exp = (s1 as u32).saturating_sub((s2 + s3 + 1) as u32);

With s1 = -5, s1 as u32 is 4294967291, so r_exp comes out around 4.29e9. p2 as u32 + r_exp then clears the precision check on line 103 and the BigInt branch evaluates BigInt::from(10).pow(4294967279). That's an unbounded allocation rather than an overflow panic, which is a considerably worse thing to hit in production than a crash. I deliberately didn't run this one.

The CometDivide and CometIntegralDivide guards are definitely needed. Could the comment say this instead of the generic overflow story? And should decimal_div reject a negative scale up front with an error, so that if anything ever does reach it the failure is diagnosable rather than a hang?

The arithmetic test can't tell a working guard from one that never fired

The cast test pins its routing properly with assertCodegenRan. The arithmetic one, "arithmetic on negative-scale decimal falls back and returns correct results", only uses checkSparkAnswer, which passes either way. Given that two of these five guards turn out not to be needed, that test would look identical whether the guard fired or not. Could it use checkSparkAnswerAndFallbackReason with negScaleDecimalArithmeticReason? That makes it assert the thing the PR is actually about.

The two deferred items

I'm fine with consolidation being a follow-up, but could you file the issue and link it here before this merges? Otherwise it won't happen. After this lands there are six scale < 0 sites carrying five different reason strings, counting the two in CometCast, the new one in MathBase, CometRound, and the one CometCeil and CometFloor share at math.scala:45 and :69.

Kernel hardening deserves the same treatment, and nothing tracks it today. 10_i128.pow(scale as u32) is still live in numeric.rs at :466, :508, :543, :569, :757 and :1309, reachable from the Iceberg native scan, a UDF, or any serde added later. One issue covering both is fine by me.

While you're filing, CometRound already routes this exact negative-scale condition through the JVM codegen dispatcher rather than falling back to Spark, which is precisely the gap #5572 is about. A child issue there for the arithmetic serdes would be worth having. Worth noting in it that mixing in CodegenDispatchFallback would also route mathDataTypeSupportLevel's unsupported-datatype case through the dispatcher, so it isn't a one-line change.