peter-toth commented on PR #58943: URL: https://github.com/apache/spark/pull/58943#issuecomment-5764600703
Thanks for going through it at that depth, @dongjoon-hyun. All 15 taken, and the transitivity point is in the description now. Pushed in `90cf67c`. **1.** The assertion was validating an `AdaptiveSparkPlanExec`, so it checked nothing. It validates the join subtree now, the way `KeyGroupedPartitioningSuite:5528` does, with the reason in a comment. **2.** Both failures are in the user-facing section, named, with what makes them reachable. **3.** Kept the delegation, and the invariant is now part of the condition rather than an assert: `partitioning.expressionsDescribeKeys && other.partitioning.expressionsDescribeKeys &&` in front of the declared-keys comparison, which is the only thing it is needed for. That is your "keep the explicit refusal" option, and I picked it over the assert deliberately: the combination is reachable only if both sides carry reduced keys with the same pairing and one is marked, and there the old answer was a shuffle, so a producer the argument misses should cost a shuffle rather than the query. `expressionsDescribeKeys` is the existing accessor for exactly this. **4.** Rewritten: "must describe one key space: two bare references, the same transform function, or the two sides of one reduce". **5.** Corrected in both places. The `@param` no longer claims a connector reducer is needed, and the fixture's scaladoc now names the property that actually makes it minimal, that it *permutes* its key space, with the reason a many-to-one transform cannot. **6.** Default dropped. `EnsureRequirements` writes `allowReduce = true`, `isCompatibleWith` writes `false`, and the test call sites say so too. **7.** Said in one line, with the `GetStructField`-is-not-a-leaf reason. **8.** New `EnsureRequirementsSuite` test. It pins which side is laid out and onto what, that the rule is idempotent over the result, and a control that two sides holding one key space are still read as they stand. It fails on base. Plain `DummySparkPlan` children, since `DummySparkPlanWithBatchScanChild` NPEs once a shuffle reads the dummy scan's table. **9.** The comment names `ShuffledJoin.clearUnknownPartitionKeys` as the operative guard now. **10.** Done, one pass: `val unknownKeys` is hoisted and the loop asks `allowReduce && !unknownKeys`. **11.** `canReduceKeys` sits behind the short-circuit at both use sites, so the two-references arm reads no configuration. Dropping the local `def` altogether made that clearer than a comment. **12.** The fixture moved to `sql/catalyst/src/test/.../catalog/functions/FlipLowBitFunction.scala`, and `InMemoryBaseTable` calls `produceResult` instead of repeating `^ 1`. Its arm keeps the neighbours' typed pattern, so a NULL key fails loudly at insert rather than reaching the function. **13.** `SimpleFunction`, so the `Unbound` wrapper is gone. **14.** `ShuffleSpecSuite` uses the real function, so the inline twin is gone. **15.** Retitled `SPARK-59688:` and moved after the last `SPARK-59045:` test. Two things I did not take, both yours to call. A simplify pass argued the boolean should be two named methods instead, mirroring this file's `keysSatisfy` / `keysCanSatisfy` pair, on the grounds that `allowReduce` is the only `Boolean` parameter in `partitioning.scala`. I kept the parameter: it is the shape you reviewed, and it keeps the change to two call sites per branch, which matters for the backport. Say if you would rather have the names. The same pass wanted the invariant enforced at the producer, a `require(!mayContainUnknownPartitionKeys || expressionsDescribeKeys)` in `KeyedPartitioning`'s body, rather than answered at the reader. I left it here, because `PartitioningCollection.fromPartitionings` ORs the marker onto a canonical layout without looking at the expressions, so a `require` there would turn a latent nothing into a construction-time throw on a path this PR does not touch. Worth its own change if you think the invariant should be enforced rather than stated. -- 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]
