andygrove commented on PR #5854: URL: https://github.com/apache/datafusion-comet/pull/5854#issuecomment-5780166239
Heads up that I approved #5846 and it should land ahead of this one, so this will need a rebase. It is the smaller of the two and it fixes silent wrong results, so I did not want to hold it behind the policy-capture question here. The overlap resolves cleanly in one direction. The `maps.scala` nested guard is now identical in both branches, so that hunk will conflict but the resolution is a no-op. The part worth being deliberate about is `native/spark-expr/src/map_funcs/map_from_arrays.rs` and the `"map"` arm #5846 adds to `comet_scalar_funcs.rs`. Because this PR renames the native call to `map_from_arrays` and replaces `MapFunc` with your own builder, both of those become unreachable the moment it lands. Could you delete them on the rebase rather than reconcile them? `validate_map_from_arrays` already does the same per-row length check, skips rows where either array is null for the same reason, and raises the same `SparkError::MapKeyValueDiffSizes`, so nothing is lost. One thing is worth carrying over rather than dropping. #5846 asserts the length mismatch in `CometMapExpressionSuite` with `checkSparkError(df, "_LEGACY_ERROR_TEMP_2128")`, which compares the exception class, the error class and the SQLSTATE across both engines and fails if a `CometNativeException` leaks through anywhere in the cause chain. The `query expect_error(must have the same length)` form in `map_from_arrays.sql` only substring-matches the message, so it would still pass on a generic native error. Could we keep the stronger assertion alongside the SQL file test? -- 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]
