Skip to content

refactor: use Arrow decimal precision validation - #5160

Open
Smallfu666 wants to merge 1 commit into
apache:mainfrom
Smallfu666:internal/issue-5089-decimal-precision
Open

refactor: use Arrow decimal precision validation#5160
Smallfu666 wants to merge 1 commit into
apache:mainfrom
Smallfu666:internal/issue-5089-decimal-precision

Conversation

@Smallfu666

Copy link
Copy Markdown

Closes #5089

What changed

  • Removed Comet’s duplicated is_valid_decimal_precision helper.
  • Updated decimal SUM and AVG accumulators to use Decimal128Type::is_valid_decimal_precision from Arrow.
  • Removed imports that were only required by the duplicated implementation.

Why

Comet now depends on Arrow 58.4.0, which exposes the equivalent boolean precision-validation API. Keeping a local copy creates unnecessary duplication and risks the implementations diverging in the future.

Validation

  • cargo fmt --check
  • cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warnings
  • cargo test -p datafusion-comet-spark-expr
  • CometCastSuite
  • CometExecSuite

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending CI. Thanks @Smallfu666

@Smallfu666

Copy link
Copy Markdown
Author

The one CI failure here — PR Build (macOS) / macos-14/Spark 4.0, JDK 17, Scala 2.13 [scans] — is #5023, not a failure of this PR.

It's a JVM SIGSEGV at pc=0x0 (no test assertion failed anywhere in the log), and symbolizing the hs_err log from that job points at hdfsThreadDestructor: it calls (*env)->GetJavaVM on a stale JNIEnv, reads a null function entry, and branches to address zero. That's the long-standing HDFS-16021, still open upstream and still present in Hadoop trunk. It only reproduces on macOS because hdrs is enabled with vendored under [target.'cfg(target_os = "macos")'.dependencies], and ParquetReadFromFakeHadoopFsSuite deliberately routes fake:// through libhdfs (COMET_LIBHDFS_SCHEMES = "fake,hdfs").

That path is disjoint from this PR: the changes are three files under native/spark-expr/ (sum_decimal.rs, avg_decimal.rs, utils.rs), all decimal aggregation, with no scan/parquet/hdfs code touched. The Linux jobs and both Spark SQL test matrices are green.

Could a committer re-run that job? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove is_valid_decimal_precision duplicated from arrow-rs

2 participants