metanil commented on PR #55885: URL: https://github.com/apache/spark/pull/55885#issuecomment-6305985451
@peter-toth Thanks for #59165. Merged master with #59165. Three quick things: 1. Your helpers needed adapting to literal parameters in children (31d38b9): `argumentsAreColumns` and `argumentShape` now look only at column arguments, and `rebuiltOver` and the `GroupPartitionsExec` rebuild go through `rewriteColumnSlots`, so they keep the literal parameters. `argumentsAreColumns` stays on bare `Attributes`, as you asked. 2. On the identity, I went with a third option rather than the `Option` or the total `functionId` (6a76da7): an expression argument is identified by the same normalized shape as `argumentShape`, via a new `ArgumentShape.Expr`, and `ColumnRef` is now bare `Attributes` only. So `bucket(4, b + 1)` is the same function as `bucket(4, c + 1)` and not as `bucket(4, x)`, and identity and the gate agree on every pair. On `master` today `isSameFunction` says `true` for `bucket(4, b + 1)` vs `bucket(4, x)` and the gate is what refuses the pair; here neither does. Only an argument over two columns, or a non-deterministic one, leaves a transform with no identity. I didn't find any path in `master` that asks `isSameFunction` without your gate, so this is about the two not depending on each other (not a bug). 3. `sameArgumentLayout` accepts two arguments of the same shape, so `bucket(8, b + 1)` can still reduce onto `bucket(4, c + 1)`, which your same-shape test checks. -- 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]
