viirya opened a new pull request, #7186: URL: https://github.com/apache/datafusion-comet/pull/7186
## Which issue does this PR close? Closes #7183. ## Rationale for this change Some JNI entry points convert their arguments with `unwrap()` / `expect()`, so a malformed argument panics. `try_unwrap_or_throw` turns that panic into a `CometNativeException`, but the exception carries only the panic message, such as ``called `Result::unwrap()` on an `Err` value: ...``, without saying which argument was bad. These conversions should return regular errors, the same way the other arguments of these entry points already do. ## What changes are included in this PR? All changes are in `native/core/src/execution/jni_api.rs`. Each conversion now returns a `CometError::Internal` through `?`, so the exception class stays `CometNativeException` and only the message changes: - `convert_datatype_arrays`, used by `columnarToRowInit`, `writeSortedFileNative` and `writeSortedPartitionsNative`, now decodes each type with a new `deserialize_arrow_datatype` helper. When decoding fails, the error says which column it was, for example `Cannot deserialize the data type of column 2: ...`. - The `file_path` and `compression_codec` arguments of `writeSortedFileNative`, and the name arguments of `traceBegin`, `traceEnd` and `logMemoryUsage`, are now read with a new `string_arg` helper. When a string cannot be read, for example because it is null, the error names the argument and the entry point, for example `Cannot read argument 'event' of traceBegin: ...`. - `get_execution_context` now returns an error for a null handle (`Native execution context handle is null`). Its callers are `executePlan`, `getShufflePartitionOffsets` and `setShufflePartitionPusher`. `setShufflePartitionPusher` already rejected a zero handle with a `NullPointerException` before reaching this check, and it still does. A data type that decodes but is invalid, such as an unknown type id, can still panic in `serde::to_arrow_datatype`. That function is shared with the planner and has many callers, so making it fallible is left out of this PR. ## How are these changes tested? New Rust unit tests in `jni_api.rs`, none of which need a JVM: - `deserialize_arrow_datatype_decodes_a_serialized_type` - `deserialize_arrow_datatype_rejects_malformed_bytes` - `get_execution_context_rejects_a_null_handle` The two error tests also check that the error is `CometError::Internal`, which maps to `CometNativeException`. The string conversions need a JVM to call, so no new tests cover them. They are covered by the existing tests of `CometNativeColumnarToRowSuite` and `CometShuffleSuite`. 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]
