metanil commented on PR #55885:
URL: https://github.com/apache/spark/pull/55885#issuecomment-6538947387
@peter-toth thanks for the review.
- **24, 25**: `f595674`, as suggested: the literal-type check is asked only
before the generalized
overload, a mismatch counts as unimplemented so the fallbacks still apply,
and the deprecated
overload is offered only to the `bucket(n, col)` layout. Its javadoc now
says Spark may offer it to
such a pair, so an implementation should check `otherBucketFunction`.
Tests cover both shapes from
25 and the width-last truncate from 24, and each fails with its part
reverted.
- **26**: `78950cd`. The four tests now run under the confs the gate test
uses, and all four fail with
the filter removed.
- **27**: `650b7f4` names the cogroup case in the migration entry. I'll
handle the pairing itself in
#59760, since it shares the gate with `canEvaluate` there.
- **16**: description updated, including the `allowCompatibleTransforms`
requirement for reducible
widths, the two migration entries, and the test count and suites.
Also closed from the earlier rounds:
- **18**: `6a1d328`. Each generalized call gets its own copies of the
parameter arrays. The copy is
shallow and a literal value can be mutable (`Decimal`), so the javadoc
still asks implementations not
to modify the values.
- **19** (the `argumentShape` half): `4796490`. `argumentShape` is built
from `expressionShape` now.
- **23**: `46533b6`. The javadoc says types are checked at positions
`inputTypes()` declares, and that a
literal past the declared arity is passed as it is, which arity-flexible
functions rely on.
- **17**: `af726ed`, as suggested.
- **21**: resolved by 25. With the literal-type check moved, the `DateType`
pair reaches
`isSingleInt`'s type clause, and the test fails with the clause removed.
- **22**: `0f5f590`
And found while going over these:
- `e7bc2db`: argument types are compared ignoring nullability. An equi-join
lets its keys differ in
`containsNull`, so the identity reduce refused an `array<int>` column
without nulls that the one-side
shuffle accepted.
Still open: 15. I'll take it (the reducer logging) as its own change, since
finding 25 widened it: a
literal type mismatch now also lands in the "implements no reducer" message.
One note on overlap with #59760, which now covers SPARK-60027 (`f82b578`):
its `evaluableFunction`
checks the arguments on the same two paths as this PR's
`argsMatchInputTypes` (the identity reduce,
and the one-side shuffle from finding 11), and more leniently, widening
integral arguments and
accepting types stored the same way. Once it lands, this PR drops its own
column check in favor of
that one, extended to the literal parameters, and the exact-type migration
entry and its cogroup
sentence go with it, since #59760 keeps the cogroup pairing. The
literal-type check before the
generalized overload (finding 25) stays here.
--
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]
