viirya opened a new pull request, #6435:
URL: https://github.com/apache/datafusion-comet/pull/6435
## Which issue does this PR close?
Part of #6434.
## Rationale for this change
The native JNI entry points mix JNI argument conversion, the native logic,
and raising the JVM
exception on failure in one function body. So the native logic cannot be
unit-tested or
benchmarked without a JVM, even where it never calls back into the JVM, and
the mapping from
`CometError` / `SparkError` to the JVM exception can only be tested through
a JVM.
This is the first step: the error classification plus a few entry points.
## What changes are included in this PR?
- `native/jni-bridge/src/errors.rs`: `throw_exception` is split into
`NativeError::from_comet_error`, which classifies an error into the JVM
exception it surfaces
as without using JNI, and `throw_native_error`, which throws it. Every
branch of the previous
code maps to one `JvmException` variant (`New`, `Spark`, `Rethrow`), so
exception classes and
messages are unchanged. All entry points go through it via
`try_unwrap_or_throw`, whose
signature is unchanged.
- `NativeStatus` gives the outcome a stable code, and
`NativeError::to_payload` serializes the
classification as a versioned JSON document (exception class, exact
message, parsed Spark
error, backtrace, and the Rust `source()` chain for diagnostics).
- `catch_native` is the panic-catching boundary without JNI.
- `native/core/src/lib.rs`: `isFeatureEnabled` and
`isObjectStoreSchemeSupported` call the new
`is_feature_enabled(&str)` and `is_object_store_scheme_supported(&str)`.
- `native/core/src/execution/jni_api.rs`:
- `decodeShuffleBlock` and `decodeShuffleBlockWithValidation` call
`decode_shuffle_block(&[u8], &[i64], &[i64], Option<&[DataType]>)`, and
`createRemoteShuffleDecoder` calls
`RemoteShuffleDecoder::try_new(&[u8])`.
- `prepare_output` now only pins the address arrays and calls
`export_batch`, which `executePlan`
also uses through it.
- `sql_error_propagation.md` is updated for the new classification step (the
previous snippet also
referred to an old path).
The plan entry points (`createPlan`, `executePlan`, `releasePlan`,
`setShufflePartitionPusher`)
depend on JVM callbacks and are not changed here.
## How are these changes tested?
- New Rust unit tests without a JVM for each classification path: Spark
errors directly and
through DataFusion wrappers, file-read classification, typed classes
through `Context` / `Shared`
wrappers, `to_exception` classes, the message-based fallbacks, panic
backtrace formatting, the
payload shape, and `catch_native`.
- A new JVM-backed Rust test that a Java throwable captured during an upcall
is rethrown as the
same exception.
- New Rust unit tests for `decode_shuffle_block` (local and remote blocks,
no output columns,
mismatched types, truncated blocks, invalid schema), `is_feature_enabled`
and
`is_object_store_scheme_supported`.
- Existing JVM-backed tests in `errors.rs` (panics, typed exceptions through
DataFusion wrappers,
`NumberFormatException`, ...) pass unchanged.
- Ran `CometNativeShuffleSuite`, `CometCelebornShuffleReaderSuite`,
`ParquetReadFromFakeHadoopFsSuite`, `SparkErrorConverterSuite`,
`CometNativeReaderSuite`,
`CometRegExpJvmSuite` and `CometExecSuite` locally.
This pull request and its description were written by Isaac.
--
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]
