Abhishek-Mohan opened a new pull request, #5958: URL: https://github.com/apache/datafusion-comet/pull/5958
> [!IMPORTANT] > **WIP / not ready to merge:** blocked on consuming a published DataFusion release containing apache/datafusion#24638. The manifests and lockfiles still use DataFusion 55.1.0, which does not contain that fix. The new Spark 3.x `pushdown=true` regression is expected to fail with the currently pinned dependency. It passes against DataFusion main plus the Comet changes below. ## Which issue does this PR close? Part of #4354. Intended to resolve it once the dependency is updated, the remaining upstream test skips are removed, and final validation passes. ## Rationale for this change Reproduction against Spark 3.4.3 and 3.5.9 found two failures behind the skipped tests: 1. Comet accepts decimal scale widening on Spark 3.x, where Spark's vectorized reader rejects it. This causes `SPARK-34212 Parquet should read decimals correctly` to fail before its exception assertions can succeed. 2. A schema-conversion error raised inside a pushed Parquet predicate loses its type in DataFusion 55.1.0. The upstream fix in apache/datafusion#24638 preserves it inside the source chain, but Comet also needs to recover the nested structured Spark error. The existing Spark 3.x shim wrapper is correct: the tagged Spark tests use `getCause.getCause`. Spark 4+ exposes the typed cause at `getCause` because its scheduler passes structured errors through. Removing the shim wrapper as suggested in the issue would break this compatibility. ## What changes are included in this PR? - Reject decimal scale increases when the existing Spark-version-specific `allow_type_promotion` flag is false. Defer this rejection until a nonempty batch is evaluated, preserving empty-file behavior and Spark 4+ widening. - Walk the native error source chain before generic JNI error classification, recovering `SparkErrorWithContext` or `SparkError` through DataFusion/Arrow/Parquet wrappers. - Strengthen the BINARY-to-decimal regression to compare the full exception-class chain and terminal message with vanilla Spark, with an explicit native-scan assertion. - Add converter, decimal compatibility, pushed-filter, and native error-extraction regressions. - Regenerate `dev/diffs/3.4.3.diff` and `dev/diffs/3.5.9.diff` from matching patched Spark sources to re-enable SPARK-34212. Restore the original typed assertion in the row-group test; its ignore remains until the dependency update. Before marking ready: - [ ] Consume a published DataFusion release containing apache/datafusion#24638. The next major release is tracked in apache/datafusion#24461 (56.0.0, planned for October); verify actual inclusion when available. - [ ] Remove the remaining row-group test ignores by regenerating the Spark diffs. - [ ] Re-run the affected native/JVM/Spark tests against the final pinned dependency and complete CI. Related but independent findings are outside this change: Spark 3.x decimal physical-width crossings, sufficiently wide unannotated integer-to-decimal reads, and accurate native offending-file attribution. ## How are these changes tested? Local environment: macOS arm64, Homebrew Rust 1.98.1, Homebrew JDK 17.0.20.1. **With released DataFusion 55.1.0 (decimal fix and initial regressions):** - All 73 native schema-adapter tests passed, including version-dependent decimal widening and empty files. - 20 targeted JVM tests passed on each Spark profile: 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. These runs preceded the final JNI source-chain change and the additional pushdown-enabled regression. - SPARK-34212 passed in upstream `ParquetV1QuerySuite` and `ParquetV2QuerySuite` on both Spark 3.4.3 and 3.5.9 (two tests per version). - The pushdown-enabled Spark 3.5 regression reproduced `SparkException -> CometNativeException` instead of Spark's typed chain. **With unmodified DataFusion main at `cee7bae63ba1ba1213b2c4aaf3bb22d803f50bda`, using temporary local Cargo overrides:** - Updating DataFusion alone still failed the pushdown-enabled regression. Adding the Comet source-chain extraction made it pass. - All 30 JNI-bridge tests passed, including wrapped structured-error recovery and a negative message-only classification test. - 21 targeted JVM tests passed on Spark 3.5, including row-filter pushdown off/on. - Both row-filter pushdown modes passed on Spark 3.4 (two targeted tests). - The original upstream row-group test passed in both V1 and V2 suites on Spark 3.4.3 and 3.5.9, after temporarily removing the ignores in the Spark checkouts (two tests per version). - V1 native scanning is asserted in the Comet tests; upstream V2 runs cover Spark fallback. Representative commands from Comet root (native built first): ```sh ./mvnw test -Pspark-3.5 -Dtest=none -Dsuites="org.apache.comet.SparkErrorConverterSuite,org.apache.comet.parquet.ParquetReadV1Suite Parquet schema conversion,org.apache.comet.parquet.ParquetReadV1Suite native scan rejects BINARY" ``` From `native/`, with the temporary DataFusion override for the main-based experiment: ```sh cargo test -p datafusion-comet parquet::schema_adapter::test cargo test -p datafusion-comet-jni-bridge ``` From a matching patched Spark checkout, with the matching Comet artifact installed: ```sh JAVA_HOME="$(/usr/libexec/java_home)" NOLINT_ON_COMPILE=true ENABLE_COMET=true ENABLE_COMET_ONHEAP=true build/sbt "sql/testOnly org.apache.spark.sql.execution.datasources.parquet.ParquetV1QuerySuite org.apache.spark.sql.execution.datasources.parquet.ParquetV2QuerySuite -- -z SPARK-34212" JAVA_HOME="$(/usr/libexec/java_home)" NOLINT_ON_COMPILE=true ENABLE_COMET=true ENABLE_COMET_ONHEAP=true build/sbt "sql/testOnly org.apache.spark.sql.execution.datasources.parquet.ParquetV1QuerySuite org.apache.spark.sql.execution.datasources.parquet.ParquetV2QuerySuite -- -z \"row group skipping doesn't overflow\"" ``` Formatting/Spotless and `git diff --check` pass. The final Spark 3.4 main-based Maven test/install used offline mode and `-Denforcer.skip=true` after the dependency check stalled; earlier released-dependency runs passed with enforcer enabled. DataFusion main also introduces an unrelated deprecated hash-join API warning. This is targeted verification, not a complete DataFusion-main upgrade validation. No local paths or experimental dependency lockfile changes are included in this PR. AI assistance: OpenCode assisted with investigation, implementation, tests, and this description, including independent agent reviews. Test results above were executed locally through the tool. -- 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]
