0lai0 opened a new pull request, #7087: URL: https://github.com/apache/datafusion-comet/pull/7087
## Which issue does this PR close? Closes #6973 ## Rationale for this change The native planner panics in three places where it should return an `ExecutionError`. The issue reports the first two. The third sits in the same function as the second. The panic is caught at the JNI boundary, so the query already fails rather than the executor crashing, but the user sees a panic message and backtrace instead of a planning error. None of these is reachable from Spark today: - Spark decimals have at most 38 digits of precision, which always fits in i128. - Only `+ - * /` reach the checked arithmetic branch. - `return_type` is always set for binary arithmetic, because `supportedDataType` only admits numeric types, and those always serialize. This is a defensive fix. It also makes the list literal path consistent with the scalar decimal literal path, which already uses `?`. ## What changes are included in this PR? - `literal_to_array_ref`: the Decimal128 arm collects into `Result<Vec<_>, _>` and propagates the error with `?` instead of calling `unwrap()` on it. - `create_binary_expr_with_options`: the `todo!` for operators other than `+ - * /` in Try/Ansi mode is replaced with a `GeneralError` that names the operator, eval mode, and data type. The old message said "ANSI" even though Try mode also reaches this branch. - `create_binary_expr_with_options`: the three arms that read `return_type` (wide decimal, decimal `Divide`, and the generic arm) called `unwrap()` on it. They now share one local closure that returns a `GeneralError` when it is missing. The field is optional on the wire because the serde sets it through `serializeDataType(dataType).foreach(...)`. `supportedDataType` makes this unreachable today, so it is the same defensive fix as the two paths in the issue. The conversion is not hoisted above the `match` because the `date_sub` arm never reads `return_type`. The wider `unwrap` cleanup in `planner.rs` is out of scope and tracked in #7086. - The optional refactor of `literal_to_array_ref` mentioned in the issue is left for a separate PR to keep this diff small. ## How are these changes tested? Four new unit tests in `planner.rs`: - `test_literal_to_decimal_list_out_of_range`: a 17-byte value (2^128) returns `GeneralError` instead of panicking. - `test_literal_to_decimal_list_with_nulls`: null slots (empty bytes with a false null mask) still decode next to valid values after the change to `collect::<Result<...>>`. - `test_checked_binary_expr_unsupported_operator`: `Modulo` with an Int32 return type returns `GeneralError` in both Try and Ansi mode. - `test_binary_expr_missing_return_type`: a binary expression with no `return_type` returns `GeneralError` in each of the three arms. `Int32` with `Plus` covers the generic arm, and `Decimal128(38, 0)` with `Plus` and with `Divide` covers the two decimal arms. Reverting any one arm to `unwrap()` makes the test panic at that arm. The out-of-range, unsupported operator, and missing return type tests fail with the original panics when the fix is reverted. `cargo test -p datafusion-comet` and `cargo clippy --all-targets --workspace -- -D warnings` pass. -- 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]
