sunchao commented on code in PR #6455:
URL: https://github.com/apache/datafusion-comet/pull/6455#discussion_r4155449532
##########
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:
[P2] Preserve Spark’s overflow timing when a native parent consumes this
output. Using the new catalog fixture with `i = 100000000`, `SELECT
decfn.ns.as_money(i) IS NULL FROM t` should return `false`: the function
returns a non-null Decimal, and Spark evaluates `IsNull` before any decimal row
write. Comet dispatches the child separately, so this new `setNull` makes
native `IS NULL` return `true`. The base writer kept the child non-null, so
this introduces a wrong result and changes filtering behavior in both ANSI
modes. Could affected expression trees remain together through the Spark
materialization boundary, or fall back when that cannot be preserved? Please
add this parent-expression regression alongside the direct-output tests.
Evidence: A local probe compiled `CometBatchKernelCodegenOutput.scala` from
both supplied SHAs and Janino-compiled its emitted writer. For an Invoke
returning `Decimal(v.toLong, 10, 0)` as DECIMAL(10,2), Spark 4.1.3
`UnsafeProjection(IsNull(call))` returned false at v=±100000000. The base Arrow
output was non-null; the head output was null. Results matched under ANSI
true/false. Spark’s direct decimal projection returned null, confirming the
materialization distinction. `CometIsNull.convert` serializes its child
separately, and native `IsNullBuilder` uses DataFusion’s `IsNullExpr` over that
child’s validity.
--
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]
