You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "peter-toth (via GitHub)" <gi...@apache.org> on 2026/09/21 11:46:24 UTC
[PR] [SPARK-59688][SQL] Fix wrong results when a storage-partitioned join skips the reduce on coinciding partition keys [spark]
peter-toth opened a new pull request, #58943:
URL: https://github.com/apache/spark/pull/58943
### What changes were proposed in this pull request?
`KeyedShuffleSpec.isCompatibleWith` answers whether two key-grouped children are lined up as they stand, so that a join can pair their partitions up index by index with no shuffle and no `GroupPartitionsExec`. It ends in `KeyLayout.describesSameKeys`, which compares the two sides' partition key rows and their types. That comparison only says what it looks like it says when both sides' keys are values of the same thing.
The predicate it consults first, `areKeysCompatible`, is deliberately looser than that. Under `spark.sql.sources.v2.bucketing.allowCompatibleTransforms.enabled` its `isExpressionCompatible` admits an `AttributeReference` against a `TransformExpression`, and two different but reducible transforms, because `EnsureRequirements` reduces such a pair onto one key space before it pairs the partitions up. So `isCompatibleWith` was reading equal key rows as "lined up" for two sides whose keys are not in one space yet.
This PR gives `areKeysCompatible` an `allowReduce` parameter, threaded into `isExpressionCompatible`, and has `isCompatibleWith` ask with `allowReduce = false`:
- `EnsureRequirements` keeps the loose question where it selects the member pair to plan on (`agreeingPairs`), because it is the caller that runs the reduce.
- `isCompatibleWith` gets the strict one, so a pair that still needs reconciling is not reported as compatible as it stands. `compatibleAsIs` is then false, and the join takes the push branch that computes the reducers and regroups both sides onto the merged keys.
- A pair an earlier join reduced together is unaffected: those keys are in one space already, and `isExpressionCompatible`'s reduced-keys arm answers for them through `hasSameReducedKeys` whatever `allowReduce` says.
- The unknown-partition-keys branch inside `areKeysCompatible` already required this same single-key-space property before comparing key subsets, through a hand-written matcher with the same reasoning in its comment. It now calls the shared predicate with `allowReduce = false` rather than repeating it. The predicate's reduced-keys arm cannot fire there, and the comment says why.
### Why are the changes needed?
Wrong results. Take a table partitioned by `identity(id)` and one partitioned by a connector transform that reorders its key space, say `flip_low_bit(id) = id ^ 1`, with ids 0 and 1 in both:
```sql
SELECT t1.id, t1.data, t2.data FROM t1 JOIN t2 ON t1.id = t2.id
```
Both scans report the partition key list `[0, 1]` of `LongType`, so `describesSameKeys` holds, but the rows behind a key differ: the identity side's key 0 holds `id = 0` while the transform side's holds `id = 1`. The join pairs partition 0 with partition 0 and finds no match in either pair, so it returns **0 rows instead of 2**, with no shuffle and no `GroupPartitionsExec` in the plan. The new end-to-end test measures exactly this on `master`.
The `Reducer` contract is what the fast path implicitly assumed more of. It says `r(f1(x)) = f2(x)`, and nothing about `r` leaving alone the keys it is applied to, so on a one-side reduce key `k` can belong with key `r(k)` on the other side. A modulo-style reducer, which is the shape a bucket count reduce takes, does leave a coinciding key alone, which is why this has stayed latent: for `bucket(8)` against `bucket(4)` the key lists can only coincide on ids below 4, where the reduce is the identity. It takes a reducer that permutes its key space to turn the dishonest answer into wrong rows.
The identity-versus-transform arm came in with SPARK-56182, which is on `branch-4.2`, `branch-4.3`, `branch-4.x` and `master`, so this is not a `master`-only bug.
### Does this PR introduce _any_ user-facing change?
Yes, it is a bug fix. The query above returns its 2 rows instead of 0, and it still runs without a shuffle: the pair goes through the reduce, so the plan gains a `GroupPartitionsExec` on each side and the identity side's keys are reduced onto `flip_low_bit`. No configuration changes, and no public API changes. `areKeysCompatible` is `private[sql]`-reachable catalyst code and the new parameter has a default, so every existing caller is unchanged.
Two plan changes are worth stating, both on pairs whose key lists coincide:
- Where the coincidence was benign, the join now reduces rather than pairing raw keys, and the reducing side reports the target transform instead of its own. A downstream operator therefore sees the coarser claim, for instance `bucket(4, id)` where it used to see `bucket(8, id)`.
- A co-partitioned operator that is not a sort-merge or shuffled-hash join, a cogroup for instance, has no reduce branch at all: `pickCoPartitionTarget` pairs children on `isCompatibleWith` alone. Such a pair is now shuffled onto one side instead of being read as it stands. That is the right answer where the reducer permutes the key space and a lost optimization where the coincidence is benign, and the `Reducer` API gives Spark nothing to tell the two apart with.
### How was this patch tested?
Two new tests, and both fail on a revert of `partitioning.scala` alone:
- `KeyGroupedPartitioningSuite`, end to end: `identity(id)` against `flip_low_bit(id)` with ids 0 and 1 in both tables. On the base it returns 0 of 2 rows; with the fix it returns both, with 0 shuffles, one `GroupPartitionsExec` per side, exactly one of them carrying a reducer, and `ValidateRequirements.validate` accepting the plan. `flip_low_bit` is a new test fixture, the smallest transform that is type-preserving and not the identity on values, which is what lets its keys coincide with a raw column's while the rows behind a key differ. No existing fixture has that shape: `bucket`, `days`, `years` and `signed_zeros` all change the type, so such a pair is refused on the key types alone, and `string_self` and `truncate` are the identity on any data whose key lists coincide.
- `ShuffleSpecSuite`, at the spec level: both shapes the parameter gates. An identity side against that transform, and two bucket counts a reducer reconciles, are each admitted by `areKeysCompatible` and refused by `isCompatibleWith`, in both directions. Two positive controls keep it honest: the two layouts do describe the same keys, and one function over one key list is still compatible with itself. A fourth case pins that a pair reduced together stays compatible, which is what keeps a chained storage-partitioned join from shuffling. The suite's `FakeBucket`, a real `ReducibleFunction`, is hoisted to the suite so both tests use it.
`build/sbt 'catalyst/testOnly *ShuffleSpecSuite'` 31 pass. `build/sbt 'sql/testOnly *KeyGroupedPartitioningSuite *EnsureRequirementsSuite *ValidateRequirementsSuite *ProjectedOrderingAndPartitioningSuite *KeyGroupedPartitioningRuntimeFilterSuite *KeyGroupedPartitioningCatalystRuntimeFilterSuite'` 318 pass. `dev/lint-scala` clean.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59688][SQL] Fix wrong results when a storage-partitioned join skips the reduce on coinciding partition keys [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58943:
URL: https://github.com/apache/spark/pull/58943#issuecomment-5766576660
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/9daeea73f29ed1c0561fe8ffe4147fdf5ca74c2f
- merged into branch-4.x https://github.com/apache/spark/commit/149747f5302ce224ae3a798ddf43f4a3706f575a
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59688][SQL] Fix wrong results when a storage-partitioned join skips the reduce on coinciding partition keys [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #58943: [SPARK-59688][SQL] Fix wrong results when a storage-partitioned join skips the reduce on coinciding partition keys
URL: https://github.com/apache/spark/pull/58943
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59688][SQL] Fix wrong results when a storage-partitioned join skips the reduce on coinciding partition keys [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58943:
URL: https://github.com/apache/spark/pull/58943#issuecomment-5763022823
Thanks for the fix, @peter-toth. I went through this carefully and I agree with both the diagnosis and the place you landed the fix: `isCompatibleWith` really is the site that asks "are these lined up *as they stand*", and `areKeysCompatible` answers a different question. I also verified the change is monotonically stricter (`isSameFunction ⊆ isCompatible`, `canReduce=false ⊆ true`), that `canReduceKeys ⇒ v2BucketingPushPartValuesEnabled` keeps every reduce-admitted pair on the push branch, and that the rule stays idempotent across an AQE re-run.
One thing worth adding to the description: this also restores **transitivity** of `isCompatibleWith`. The old `isCompatible`-based answer was non-transitive (`bucket(12) ~ bucket(4)` and `bucket(4) ~ bucket(8)`, but `bucket(12) ≁ bucket(8)`), while `ValidateRequirements` compares every child against `specs.head` only. `isSameFunction` is an equivalence, so that latent hole closes too.
Below are the things I'd like to see addressed, most important first.
---
### 1. The `ValidateRequirements.validate` assertion is vacuous
`KeyGroupedPartitioningSuite.scala:1357`
```scala
assert(ValidateRequirements.validate(plan), "the plan that leaves must hold up")
```
`spark.sql.adaptive.enabled` defaults to `true` and neither the test nor `KeyGroupedPartitioningSuite.sparkConf` disables it, so `plan` is an `AdaptiveSparkPlanExec`, which `extends LeafExecNode`. `ValidateRequirements.validate` is `plan.children.forall(validate) && validateInternal(plan)`, and on a childless node `validateInternal` short-circuits to `satisfied = true` with the `children.length > 1` co-partitioning branch never entered. Revert `allowReduce = false` and re-add only this assertion — it still passes.
The sibling assertions survive only because `collectShuffles` / `collectGroupPartitions` go through `AdaptiveSparkPlanHelper.collect`, whose `allChildren` explicitly descends into `AdaptiveSparkPlanExec.executedPlan`; `ValidateRequirements` does not.
This suite already documents the trap and works around it — see `KeyGroupedPartitioningSuite.scala:5528-5534` ("a query stage is a leaf, so validating an AQE plan checks nothing"), which validates `collect(stripAQEPlan(...)) { case smj: SortMergeJoinExec => smj }.head` instead. Since the PR description cites this assertion as evidence, I'd fix it the same way.
### 2. Two planning-time exceptions become newly reachable
`partitioning.scala:2001`
Pairs that used to take the `compatibleAsIs` shortcut now enter the push branch. Two hard failures follow for queries that previously planned:
- `KeyedPartitioning.reduceKeys` takes the reduced types from the connector's `Reducer.resultType()`, so a connector violating the `r(f1(x)) = f2(x)` type contract now hits `storagePartitionJoinIncompatibleReducedTypesError`.
- For the identity-vs-transform arm, `IdentityReducer.reduce` calls `bound.eval(...)`, and `V2ExpressionUtils.loadV2FunctionOpt` accepts any `BoundFunction` with no `ScalarFunction` check — so a transform bound to a non-`ScalarFunction` gives `resolvedFunction = None` and raises `cannotEvaluateExpressionError`.
Both are better than the wrong rows they replace, but they are user-visible and the "Does this PR introduce any user-facing change?" section doesn't mention either.
### 3. The unknown-keys branch is loosened, not tightened
`partitioning.scala:2062`
This is the one place the diff relaxes the predicate. The deleted arm was `case (l: TransformExpression, r: TransformExpression) => l.isSameFunction(r)`; `isExpressionCompatible` now routes a marked + reduced pair to `hasSameReducedKeys`. Left `bucket(12, a).reducedTogetherWith(bucket8)` vs right `bucket(8, b).reducedTogetherWith(bucket12)` was refused before and is accepted now — and two marked sides route their undeclared rows at `hash(key) % numPartitions` in their *original* key spaces.
I traced all four marker producers and your unreachability claim does hold today. But it rests on three cooperating sites in three files (`canCreatePartitioning`'s `expressionsDescribeKeys`, `PartitionGrouping.isIdentity` being false whenever a reducer slot exists, and `ShuffledJoin.clearUnknownPartitionKeys`) with no assertion anywhere. Either keep the explicit refusal or add `assert(!hasReducedKeys(l) && !hasReducedKeys(r))`.
### 4. Rewritten comment 3.3 is factually false
`partitioning.scala:1994`
> 3.3 each pair of partition expressions at the same index must share the same transform function.
`isCompatibleWith` does not require that. The reduced-keys arm answers `hasSameReducedKeys`, which is true for differing `functionId`s: `bucket8.reducedTogetherWith(bucket4)` has `numBucketsOpt = Some(8)` and `bucket4.reducedTogetherWith(bucket8)` has `Some(4)`, so `isSameFunction` is false while `hasSameReducedKeys` is true — and `ShuffleSpecSuite.scala:757-759`, added by this PR, asserts that pair *is* compatible. The first arm also admits two plain attributes, which share no transform at all. The replaced wording ("compatible transform functions") was loose but true.
The risk is concrete: someone reading 3.3 as a guarantee and tightening the reduced-keys arm to `isSameFunction` sends every chained SPJ back to a shuffle — exactly what `ShuffleSpecSuite:756-759` exists to protect.
### 5. The `allowReduce` scaladoc overstates what triggers the bug
`partitioning.scala:2028`
> it takes a connector reducer that reorders its key space to turn this into wrong rows
For the identity-vs-transform arm the same paragraph names, there is no connector `Reducer` at all — `reducersBothWays` synthesises `IdentityReducer(t.withReference(a))` from the *other side's ordinary transform*. Your own `FlipLowBitFunction` is a plain `ScalarFunction[Long]`, not a `ReducibleFunction`, and it produces the wrong rows. All it takes is a transform that preserves its argument type and is not order-preserving.
Same overstatement in the fixture comment at `transformFunctions.scala:290-294`: "result type is its input type, and not the identity on its inputs" does not exclude the existing `TruncateFunction` (`StringType -> StringType`, not the identity). What actually makes `flip_low_bit` the minimal fixture is that it is a **permutation** of the key space; `truncate` is many-to-one, so it cannot mispair rows a raw-column join would match. Worth stating that way — it's the sharper reason.
### 6. `allowReduce` defaults to the unsafe value
`partitioning.scala:2030`
There are exactly two production call sites: `EnsureRequirements.scala:703` (wants `true`, and is the caller that runs the reduce) and `partitioning.scala:2001` (wants `false`). Every other consumer asks the as-they-stand question. A future site written as `spec.areKeysCompatible(other)` — the natural spelling — silently gets the reduce-allowed answer and reintroduces SPARK-59688 with no compile error and no failing test.
Dropping the default and writing `allowReduce = true` at the one `EnsureRequirements` call site costs a word; only `ShuffleSpecSuite`'s existing assertions need the extra argument. Note those assertions ride the default too, so flipping it later would silently reverse their meaning rather than fail to compile.
---
### Smaller items
7. **`partitioning.scala:2100`** — the delegation silently widens the marked-keys gate from `(_: AttributeReference, _: AttributeReference)` to `(_: LeafExpression, _: LeafExpression)`. Unreachable today, but this is the gate protecting the `partitionKeys` subset comparison, and reverting this hunk alone leaves every test in the diff green — `ShuffleSpecSuite:662` is the only coverage and all its inputs are `AttributeReference`/`TransformExpression`. At least say in the comment that it's a widening rather than a restatement.
8. **Cogroup path has no test.** `checkKeyGroupCompatible` returns `None` for every non-SMJ/SHJ parent, so `FlatMapCoGroupsInPandasExec` and friends reach `pickCoPartitionTarget` as their only path. With `v2BucketingShuffleEnabled=true`, that path had the same bug, and the failure mode is worse than for joins because no equi-predicate filters the mispaired groups — the join returns zero rows, a cogroup hands the user's function rows that don't belong together. This PR fixes it silently; `EnsureRequirementsSuite`'s keyed-cogroup tests (1502, 2042) use no compatible-but-different transform pair.
9. **`partitioning.scala:2049`** — the unreachability argument names `createPartitioning` as where marked layouts come from, but `PartitioningCollection.fromPartitionings` also produces them: it ORs the marker across members (line 1483) and stamps the OR'd flags onto the canonical layout, and its `require` guards `describesSameKeys` but not the marker. The operative guard is `ShuffledJoin.clearUnknownPartitionKeys`. The conclusion holds; a reader checking a future change would check the wrong site.
10. **`partitioning.scala:2060`** — when `allowReduce = false` the unknown-keys `forall` is textually identical to the outer one at 2039-2040, so it is pure duplicate work; and since `isExpressionCompatible(l, r, false) ⇒ isExpressionCompatible(l, r, true)` on every arm, the outer pass is subsumed in the `allowReduce = true` case too. Hoisting `val unknownKeys = partitioning.mayContainUnknownPartitionKeys || other.partitioning.mayContainUnknownPartitionKeys` and calling the outer loop with `allowReduce && !unknownKeys` collapses both into one pass, and states the intent (the inner loop exists only to *downgrade* a lenient caller) which isn't visible today.
11. **`partitioning.scala:2098`** — `val canReduce = allowReduce && canReduceKeys` is eager, so the `(Leaf, Leaf)` arm now evaluates `SQLConf.get` plus three conf reads where it previously read none. That's the plain `identity(id)` vs `identity(id)` shape, hit once per partition expression per candidate pair over the `leftCandidates × rightCandidates` cross product. `lazy val`, or a local `def`, restores the old cost.
12. **`InMemoryBaseTable.scala:347`** — `value ^ 1L` re-implements `FlipLowBitFunction.produceResult` with a comment as the only sync, the fourth such pair in this file. They already disagree on NULL: the `case (value: Long, LongType)` type test doesn't match null so the write path throws, while `ApplyFunctionExpression.eval` has no null guard and `getLong(0)` on a null slot returns 0, mapping NULL to key `1L`. Avoidable — moving `FlipLowBitFunction` into the existing `sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/functions/` package lets this arm call `FlipLowBitFunction.produceResult(InternalRow(value))` (`InternalRow` is already imported), and lets `ShuffleSpecSuite` use the real function too.
13. **`transformFunctions.scala:281`** — `UnboundFlipLowBitFunction` is `UnboundSignedZerosFunction` with two strings changed. `SimpleFunction` (`@since 4.2.0`) exists for exactly this, with precedent at `DataSourceV2FunctionSuite.scala:753`; `object FlipLowBitFunction extends SimpleFunction with ScalarFunction[Long]` drops the wrapper and still works with `withFunction`. The copied type check is dead anyway — `loadV2FunctionOpt` swallows the `UnsupportedOperationException` into `None`.
14. **`ShuffleSpecSuite.scala:715`** — the inline `flipFn` is behaviourally identical to the `FakeBucket` this PR just hoisted, for everything this suite observes (`resultType` and "is a `TransformExpression`"); the function is never evaluated here, so the `a ^ 1` semantics the comment explains aren't exercised. The same test builds a `FakeBucket` 29 lines later. Item 12 would let you use the real function instead.
15. **`KeyGroupedPartitioningSuite.scala:1332`** — the title has no JIRA ID (172 of 188 tests in this file are `SPARK-xxxxx: ...`; the 16 that aren't are original feature tests, not bug fixes), so `git grep SPARK-59688` won't find the regression test. It also lands between the 4th and 5th of five consecutive `SPARK-59045:` tests; moving it after the last one (before the `SPARK-59121` pair) keeps both the topical adjacency and the ticket grouping.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59688][SQL] Fix wrong results when a storage-partitioned join skips the reduce on coinciding partition keys [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59688][SQL] Fix wrong results when a storage-partitioned join skips the reduce on coinciding partition keys [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #58943:
URL: https://github.com/apache/spark/pull/58943#issuecomment-5772923448
Thank you @dongjoon-hyun for the review!
Backport PRs are here: https://github.com/apache/spark/pull/58963, https://github.com/apache/spark/pull/58964
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59688][SQL] Fix wrong results when a storage-partitioned join skips the reduce on coinciding partition keys [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58943:
URL: https://github.com/apache/spark/pull/58943#issuecomment-5766586197
Could you make a backporting PR to branch-4.3 and branch-4.2, @peter-toth ? There were some conflicts from branch-4.3.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org