parthchandra commented on PR #6455:
URL:
https://github.com/apache/datafusion-comet/pull/6455#issuecomment-5919288197
Notes (the generated code now matches Spark's
`UnsafeRowWriter`/`UnsafeArrayWriter` decimal rescale: `changePrecision`
HALF_UP, write null when it doesn't fit):
- **`spark/src/test/scala/org/apache/comet/CometCodegenSuite.scala:2423`** —
every test value has scale 0 and the declared scales are 2 and 12, so
`changePrecision` only ever pads zeros and the HALF_UP rounding branch never
runs, even though Spark's writer rounds here. Please add a function that
returns a higher-scale Decimal than it declares, e.g. `1.005` declared as
`DECIMAL(10, 2)`, so the test proves Comet rounds the same way Spark does. A
round-and-overflow case too.
- **`CometCodegenSuite.scala:2366`** (the SELECT) — the nested writer is
only exercised through `map('k', ...)`. Please add `array(fn(i))` and a struct
with a decimal field, covering a nullable and a non-nullable child, so the
array and struct decimal writers are exercised, not just the map one.
-
**`spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegenOutput.scala:234`**
— the generated decimal branch has no null check before `$dec.precision()`,
unlike Spark's writers. It's safe (the top-level write runs only in the `else`
of the `ev.isNull` guard, nested writes are behind `isNullAt`), but a one-line
comment saying the null case is handled by the caller would stop the next
reader thinking it's a missing check.
- **`CometBatchKernelCodegenOutput.scala:263`** — on a non-nullable result
that overflows, Comet fails with "declared as non-nullable but contains null
values" while Spark fails with `EXPRESSION_DECODING_FAILED`. Both fail, so no
wrong-result risk, but that divergent error path has no test. Please add one
asserting both engines error, even if the messages differ.
- **`CometCodegenSuite.scala:2431`** — the test only exercises the `Invoke`
path. `StaticInvoke` shares the same writer and the PR's scaladoc change is in
`CometStaticInvoke`, so a static-method variant would give direct 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]
