sunchao commented on code in PR #6455:
URL: https://github.com/apache/datafusion-comet/pull/6455#discussion_r4161557411
##########
spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegenOutput.scala:
##########
@@ -231,15 +231,39 @@ private[codegen] object CometBatchKernelCodegenOutput
extends CometTypeShim {
val set = if (nested) "setSafe" else "set"
OutputEmit("", s"$targetVec.$set($idx, $source);")
case dt: DecimalType =>
+ // Rescale to the declared type, and write null when the value does not
fit, as Spark's
+ // `UnsafeRowWriter` and `UnsafeArrayWriter` do in `write(ordinal,
Decimal, precision,
+ // scale)`. A Spark expression already produces its declared precision
and scale, but a
+ // DSv2 function called through `Invoke` / `StaticInvoke` can return a
`Decimal` of any
+ // scale (#6425). Like Spark's writers, this rescales the value in
place, and leaves it
+ // untouched when it does not fit. Unlike them, it does not test
`source` for null: the
+ // callers write null values themselves, and skip that test only for a
type that is not
+ // nullable.
+ //
+ // The precision and scale test repeats `changePrecision`'s own fast
path. It keeps the call
+ // off the common path, so the JIT can still scalar-replace the
`Decimal` that an input
+ // getter allocates. With the bare call, passing a `DECIMAL(18, 2)`
column through took
+ // about half as long again per row.
+ //
// DecimalOutputShortFastPath: precision <= 18 fits in a signed long, so
pass the unscaled
// value to `setSafe(int, long)` and skip the BigDecimal allocation.
+ val dec = ctx.freshName("dec")
+ val (precision, scale) = (dt.precision, dt.scale)
val write =
- if (dt.precision <= Decimal.MAX_LONG_DIGITS) {
- s"$targetVec.setSafe($idx, $source.toUnscaledLong());"
+ if (precision <= Decimal.MAX_LONG_DIGITS) {
+ s"$targetVec.setSafe($idx, $dec.toUnscaledLong());"
} else {
- s"$targetVec.setSafe($idx, $source.toJavaBigDecimal());"
+ s"$targetVec.setSafe($idx, $dec.toJavaBigDecimal());"
}
- OutputEmit("", write)
+ OutputEmit(
+ "",
+ s"""org.apache.spark.sql.types.Decimal $dec = $source;
+ |if (($dec.precision() == $precision && $dec.scale() == $scale) ||
+ | $dec.changePrecision($precision, $scale)) {
+ | $write
+ |} else {
+ | $targetVec.setNull($idx);
Review Comment:
The direct-parent cases now pass, but I reproduced @parthchandra's
transitive-consumer concern on `eb5bf55cd0a677f29f5c3c6a563c3caab9e7bf79` with
the PR's catalog fixture.
For rows `3, NULL, 100000000, -100000000`, in both ANSI modes:
- `abs(decfn.ns.as_money(i)) IS NULL` returns `true` for both overflowing
values in Comet and `false` in Spark.
- `decfn.ns.money_array(i)[0] IS NULL` and `decfn.ns.money_struct(i).m IS
NULL` show the same mismatch.
- `count(abs(decfn.ns.as_money(i)))` returns `1` in Comet and `3` in Spark.
All three nullness/count probes pass against base
`75d7c7afda78cef4894dc7a183d810c7be463aa0` using the same catalog fixture. The
five existing #6425 tests pass on the head. These are local Spark 4.1.3 / JNI
executions, with native Comet operators confirmed in the failing plans.
The intermediate expression is dispatched and writes a rescaled/null
decimal, but its native parent or aggregate does not match the immediate-child
check. Could the guard preserve the affected value through the full expression
chain, or fall these chains back, and add these nested cases to the regression
coverage?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
