metanil commented on PR #55885: URL: https://github.com/apache/spark/pull/55885#issuecomment-6538964626
> @metanil, a follow-up on the [SPARK-60027](https://issues.apache.org/jira/browse/SPARK-60027) point in finding 16 of [my last round](https://github.com/apache/spark/pull/55885#pullrequestreview-5643168257). I suggested naming the ticket so that it closes with this PR, but on second thought I'd rather split that part out. > > The exact-type check on the columns fixes a bug that released branches have. The one-side shuffle (since [SPARK-48012](https://issues.apache.org/jira/browse/SPARK-48012)) and the identity reduce (since [SPARK-56182](https://issues.apache.org/jira/browse/SPARK-56182)) evaluate a transform whose arguments differ from its `inputTypes()`. That fails with a `ClassCastException`, or returns wrong rows for a function with a magic `invoke`. For example, the test catalog's `years` over a `DATE` column returns 1 of 3 rows in a one-side shuffle on master, since its `invoke(long)` reads the raw days as microseconds. > > On master the same check applies to `numBucketsOpt` and the children, without this PR's literal parameters. So it could go into the `canEvaluate` gate of #59760, which refuses transforms Spark cannot evaluate on the same two paths, and be backported with it. This PR would then keep the literal-parameter checks the generalized reducer needs. > > One effect to decide there, by code reading (I haven't measured it). Iceberg's `bucket` declares `INT` for a `TINYINT` or `SMALLINT` column. On master Java widens the value into its `invoke(int, int)`, and the join stays shuffle-free. An exact check makes that join shuffle, which a backport shouldn't do, so integral widening may need to count as a match. > > What do you think? I'm happy to open the [SPARK-60027](https://issues.apache.org/jira/browse/SPARK-60027) PR myself if you'd rather keep yours as they are. Agreed, and it's already in #59760 now (`f82b578`): `evaluableFunction` refuses a transform whose arguments, the bucket count included, don't suit `inputTypes()`, so it can be backported with SPARK-60522. More info there. Once #59760 lands, this PR drops its own column check and keeps only the literal-parameter checks the generalized reducer needs. -- 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]
