asugranyes commented on PR #58818: URL: https://github.com/apache/spark/pull/58818#issuecomment-5686538363
Thanks for the follow-up. I went through the different execution paths and the local normalization approach looks good to me. This is also very close to the approach I initially took in #53695 before we redirected the implementation to the optimizer-rule path in `NormalizeFloatingNumbers`: handling floating-point normalization locally in the array set operations / `SQLOpenHashSet` path. Given the concerns described here with the logical-plan rewrite and the extra `ArrayTransform` pass, going back to local handling makes sense to me. Two remarks: **1. Interaction with** #53468 This PR removes the fallback paths for complex types and uses a `GenericComparableWrapper` as the hash key instead. Since `normalizedElement` is currently used when emitting values from those fallback paths, I think we should make sure the two changes compose correctly. In particular, the wrapper needs to provide hash/equality semantics consistent with the floating-point equivalence expected for nested values. Since `normalizedElement` currently also canonicalizes complex values when they are emitted, it would be good to clarify how this should compose with #53468 once the fallback path is removed. This also seems relevant for the planned `ArraysOverlap` follow-up (https://issues.apache.org/jira/browse/SPARK-58943). I don't see this as a blocker for this PR. Given the current implementation and the existing O(N^2) fallback for complex types, the approach here looks good to me. I'm noting this as an integration point to keep in mind. **2. Cost of `normalizedElement` for complex types** `normalizedElement` builds an `UnsafeProjection` over a synthetic `BoundReference`, and each emitted complex element goes through `InternalRow(Value)` -> `Projection`-> `get `-> `copyValue`. The new approach avoids the upfront `ArrayTransform` over the entire input array, which is a clear advantage. However, for complex/nested types, particularly when most input elements are eventually emitted, I wonder if it would be worth adding a small benchmark to quantify the cost of the per-element projection. -- 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]
