andygrove opened a new pull request, #4382:
URL: https://github.com/apache/datafusion-comet/pull/4382
## Which issue does this PR close?
Closes #.
## Rationale for this change
The internal `spark.comet.use.decimal128` conf is dead plumbing:
- The conf is **write-only**: three callers (`CometExecRule`,
`CometShuffleExchangeExec`, `CometNativeExec.doExecuteColumnar`) force it to
`"true"` right before native execution, but **nothing reads it anywhere** in
the repo.
- The `useDecimal128` boolean threaded through the entire `CometVector`
hierarchy is always `true` in production. Both `NativeUtil.importVector` and
`NativeUtil.rootAsBatch` hardcode `true`, so the `!useDecimal128` branch in
`CometVector.getDecimal()` is unreachable.
- The Rust `SparkParquetOptions.use_decimal_128` field is declared,
defaulted to `false` in both `::new` constructors, and never set or read
elsewhere.
This looks like legacy from the era when the JVM Parquet reader produced
Decimal32/Decimal64 vectors. With the native scan path being the only path,
every decimal column already lands in 128-bit form before reaching the JVM.
## What changes are included in this PR?
- Remove `CometConf.COMET_USE_DECIMAL_128` and its three `setConfString`
writers.
- Remove the `useDecimal128` field and constructor parameter from
`CometVector`, `CometDecodedVector`, `CometPlainVector`,
`CometDictionaryVector`, `CometListVector`, `CometMapVector`,
`CometStructVector`, `CometDelegateVector`, `CometSelectionVector`.
- Collapse `CometVector.getDecimal()` to the always-128-bit path.
- Drop the `true` arg from both `NativeUtil` callers of
`CometVector.getVector` and from the codegen template in
`CometBatchKernelCodegenInput`.
- Remove `use_decimal_128` from `SparkParquetOptions` and clean up three
stale comments in `columnar_to_row.rs` that referenced the conf.
- Update `TestColumnReader` callers to the simplified constructor.
## How are these changes tested?
Covered by existing tests:
- `ParquetReadV1Suite "decimals"`: passes (Standard + Legacy modes plus
sibling decimal tests).
- `CometShuffleSuite "fix: StreamReader should read shuffled decimal columns
as Decimal128"`: passes (renamed from the old `useDecimal128` regression test,
still pins `NativeUtil.rootAsBatch` against decimal-128 expectation).
- The `Seq(true, false).foreach { useDecimal128 => ... }` sweep in
`ParquetReadSuite "decimals"` was flattened since the conf no longer exists;
the remaining batch-size and precision matrix still exercises the decimal read
path.
- `make` and `cargo clippy --all-targets --workspace -- -D warnings` both
green.
--
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]
