zhuqi-lucas commented on issue #25572:
URL: https://github.com/apache/datafusion/issues/25572#issuecomment-5771425546
Since my last comment I built the POC end to end, and the findings changed
the picture enough that this supersedes the sequencing I proposed above. Short
version: I think there are three separable problems here, and the empirical
findings assign each one a different status. Draft PR with everything below:
#25585.
**Problem A: chains hand-repeat enforcement because there is no phase
structure.** Your diagnosis, and the POC supports it: the phase split removes
the reason most of those repeats exist. This is the structural track and it
stays the main body of this issue.
**Problem B: the once-through list depends on hand-ordering, and a loop
would fix that.** This is the convergence idea, and the POC says it is
premature, not wrong. Rerunning rules speculatively means `f(f(x))` in an
ecosystem that has only ever exercised `f(g(x))` (re-enforce after someone
else's rewrite; even the default chain's own duplicated
`ProjectionPushdown`/`FilterPushdown` entries are that pattern). Testing the
idempotence this requires falsified every rule it touched:
1. `FilterPushdown` re-application conjuncts the same predicate again (`k <
10 AND k < 10 AND k < 10`; 63 slt failures, one signature).
2. A second `PushdownSort` application returns wrong rows
(`sort_pushdown.slt:2487`: 4,5,6 for an `ORDER BY id ASC LIMIT 3` whose answer
is 1,2,3). Probably reachable today by any chain listing the rule twice; filing
separately with the repro.
3. Even `EnsureRequirements`, the one rule whose specification promises
idempotence, changed 35 plans across eight slt files when declared: a second
application rewrites sort-preserving merges to other members of the ordering
equivalence class, once replacing a `CoalescePartitionsExec` with a costlier
merge.
So in the PR the loop exists per call site behind `fn idempotent(&self) ->
bool { false }` on the rule trait, and nothing declares it: the machinery is
built, mutation-tested, and dormant, and each future declaration is gated on a
fix plus a regression test. The declaration being the rule's own method follows
the same logic that (rightly) rejected the config key in #25356: how often a
rule runs is the chain's property, but whether re-application is safe is the
rule's.
**Problem C: authored repeats that survive both A and B still mostly
re-derive settled plans.** Some downstream rules consume the operators
enforcement materializes, so at least one enforce-optimize-enforce alternation
outlives the phase split, and its enforcement calls are usually no-ops (on the
chain that motivated this: 4 of 6 calls byte-identical, 79% of physical
optimization time). The property this needs is not idempotence but determinism,
which survives everything above: a rule that oscillates is deterministic,
records no fixpoint, and is simply never skipped. Hence the second trait
method, `fn deterministic(&self) -> bool { false }`: a declaring rule handed a
plan it has been *observed* to leave unchanged in this run is skipped outright
(pointer fast path, then a full-fingerprint compare; full rather than hashed,
since a collision would skip an enforcement pass that had work to do).
`EnsureRequirements` declares this one, and it holds at every level. Upstream,
the full sql
logictest suite is green. Downstream, a live A/B on the very endpoint that
motivated this work, two server builds side by side on the same host and the
same data at the same moment, one with the declaration and one without: **the
six authored enforcement calls drop to three executed, physical optimization
wall goes from 87.0 ms to 48.5 ms (-44%), and the two builds' rendered plans
are byte-identical** (74,964 bytes each). A unit test over the same chain pins
the mechanics and one integration lesson worth writing down: instrumentation
wrappers must delegate `deterministic()`, or the skip dies silently. This is
#25356's mechanism re-homed onto the rule, where I now think it always
belonged, and it is the piece that solves the production latency problem that
started this whole thread for us: no longer a projection, measured live.
**Sequencing I would propose:** (1) `PhysicalPlanSignature` +
`deterministic()` + the skip as the first real PR, since it has a user today
and zero contract exposure; (2) the dormant `idempotent()`/convergence
machinery lands with the phase-split design work, waking rule by rule as fixes
land (`FilterPushdown` predicate dedup, the `PushdownSort` bug, canonicalizing
ER's equivalence-class rewrite); (3) the `PhysicalAnalyzerRule` split continues
here as the structural track.
@alamb what do you think of the `deterministic()` approach? If the shape
looks right to you I will polish #25585 and mark it ready for review.
--
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]
