peter-toth commented on PR #57753:
URL: https://github.com/apache/spark/pull/57753#issuecomment-5207280556
No problem at all, and thanks — this review was greatly appreciated. It
found real gaps, including a couple I'd have shipped. Everything is in
`97d5247`.
**What changed**
- **Transform comparison (`:858`).** This was the big one.
`TransformExpression` is a plain case class, so `canonicalized ==` compares the
`BoundFunction` instance, and `V2ExpressionUtils` binds afresh on every
derivation — so two identical `bucket(4, id)` reports compared unequal and
every transform-partitioned source declined the merge. Both dimensions now
compare with `isSameFunction`, recursively, so a nested transform's children
don't fall back to the instance comparison either. One fixture trap worth
flagging: Spark's `UnboundBucketFunction.bind` returns a singleton, so an
in-repo bucket test passes with or without the fix — the new test uses a
`FunctionCatalog` that binds a fresh instance per call, like a real connector,
and it fails without the fix.
- **Report checked per build attempt (`:831`, `:806`).** The check moved out
of `tryBuildMergedDSv2Scan` to its callers, and now runs per attempt. A
degradation is no longer indistinguishable from a filter-enforcement failure,
so the build's `None` again means only "strict filters not re-enforceable" —
which restores the `tryBuildFilterDSv2ScanChild` contract you flagged — and a
source that reports per scan still gets its chance from the strict-only attempt.
- **Coverage (`:877`, `:2622`).** You were right that the ordering check
never fired with a non-empty requirement; I measured it — replacing
`merged.ordering.getOrElse(Nil)` with `Nil` left all 82 tests green.
`TestV2Table` can now report an ordering or a bucket partitioning, and there
are four new tests: the two ordering ones, the bucket-transform one, and the
incompatible-inputs-with-the-config-on case, which was indeed uncovered.
- **Configs (`:7297`, `:7298`).** Both renamed to end in `.enabled`, and
both docs now mention the decline that happens before any rebuild. I dropped
the redundant `allow` while renaming, because
`allowKeyGroupedPartitioningDegradation.enabled` came to 94 chars — one over
the line limit, so it would have been the only key in `SQLConf` split across
two literals, and not greppable from a log.
- **Comments and names (`:724`, `:841`, `:2603`).** All fixed, plus two more
the same pass turned up: the `(Filter, Filter)` call site still claimed strict
filters were the only reason to decline, and `DSv2DeferredScan`'s `@param`s
named the configs by their old keys.
- **Nits (`:179`, `:104`).** The `withSQLConf` was decorative as you say —
rather than drop it I made it matter, by also asserting the merged scan reports
`KeyedPartitioning` on the partition column in the physical plan (AQE off so
the plan is walkable). The fixture is down to one `createTable` and one
`capabilities()`.
**Where I'd push back a little**
- `:872`, second half — emptying the requirement at line 730 is a no-op
rather than a lost check: reaching `getOrElse(Nil)` with a `None` implies that
dimension's config is on, and `mergeDegradesReporting` already short-circuits
on it.
- `:872`, first half — a *gained* report is real (the partitioning pass is
reference-subset guarded, so `{c1,c3}` + `{c2,c3}` does give the merged scan
`Some([c1,c2])`), but I read it as the win rather than a hazard: the source
reported that partitioning for the table, and an input dropped it only because
a pruned column left it inexpressible. I've documented it so it reads as
deliberate.
- `:867` — agreed the check sees only the expressions, and that limitation
is now written down. `keyGroupedPartitioning` is `Option[Seq[Expression]]`, so
split counts and partition values aren't visible at that layer, and planning
input partitions in the optimizer to compare them seems worse than the gap. It
isn't specific to the `.orElse` either — the first build can prune differently
from either input too.
- `:802` — a zero-key `KeyGroupedPartitioning` does now reach the merged
scan as `Some(Nil)`. But the two unmerged scans would each have carried the
same `Some(Nil)` and collapsed the same way, so it's not worse than not merging
— the old `None` was accidentally better than the original plan. Commented
rather than changed.
- `:2622`, first half — I couldn't reproduce this one: setting the equality
to `false`, and deleting the np-side `mapAttributes`, both fail
`DSv2PlanMergingSuite`'s new end-to-end test. The two subquery relations carry
different exprIds, so the remap is what makes the two reports comparable at
all. Both do stay green if `MergeSubplansSuite` runs alone.
- `:877`, second half — picking the weaker side is green only for two
non-empty orderings; flipping the empty-side branches along with it fails the
existing single-side decline test. The new test covers the non-empty case.
**Two follow-ups I'd rather not fold in here**
- `TransformExpression`'s equality is the root cause and it reaches past
this rule: `PartitioningPreservingUnaryExecNode.projectKeyedPartitionings` puts
kGP expressions in an `ExpressionSet` (so dedup silently fails and the
cross-product can hit `aliasCandidateLimit`), `BatchScanExec.equals` compares
`keyGroupedPartitioning` directly, and `DataSourceV2ScanRelation`'s canonical
form does too, so `PlanMerger.checkIdenticalPlans` can miss two identical
subqueries. Overriding `equals`/`hashCode` in terms of `canonicalName()` would
fix all of them and let me delete the helper this PR adds. I'll file a JIRA.
- The strict-only retry in `tryBuildFilterDSv2ScanChild` is untested —
deleting the `.orElse` leaves everything green — because the fixture is
all-or-nothing on filters, so both attempts fail together. That predates this
PR (SPARK-40259); it needs a source that pushes a batch or nothing.
--
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]
