[feature] Inline let-bound persistent paths for index pre-select#13
Open
joewiz wants to merge 2 commits into
Open
[feature] Inline let-bound persistent paths for index pre-select#13joewiz wants to merge 2 commits into
joewiz wants to merge 2 commits into
Conversation
A query of the form
let $v := <persistent path> return $v[Optimizable-pred]
ran ~167x slower than the equivalent direct form. The legacy Optimizer
wraps the FilteredExpression in the (#exist:optimize#) pragma in both
cases, but at runtime BasicExpressionVisitor.findFirstStep cannot find
a top-level LocationStep when the FE source is a VariableReference, so
Optimize.eval records contextStep == null and falls back to
innerExpr.eval(result, ...) -- which invokes FilteredExpression.eval,
which calls VariableReference.eval, which reads the bound value off
the local variable stack and ignores contextSequence. The pre-selected
NodeSet is therefore computed and then thrown away, and the predicate
runs once per node in the full input.
This commit adds LetInliner.tryInline(let, cc) called from
LetExpr.optimize(cc). When all six soundness gates hold:
1. variable name present and not a score binding (XQFT 3.0 §2.3);
2. standalone let, not a chain link;
3. no declared sequence type;
4. node-typed input;
5. input contains a non-wildcard LocationStep;
6. body unwraps to a FilteredExpression whose source is the variable,
with exactly one Optimizable predicate, and the variable does not
appear anywhere else in the body,
the predicate is appended to the input's last LocationStep and the
LetExpr is replaced by the input path. The legacy Optimizer pass then
sees the same shape it knows how to wrap from the direct form, attaches
the pragma to the LocationStep, and routes through the index pre-select.
Gate 6 mirrors what was noted in a 2016 comment on the issue thread:
inlining is only desirable when the inlined form would expose an
Optimizable to visitLocationStep.
Closes eXist-db#873
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six tests covering the LetInliner rewrite:
- issue873_indirectQueryReturnsSameNodes -- direct vs indirect must
return the same nodes (correctness check).
- issue873_inlineRewriteLogged -- the optimizer rewrite log must
contain an "inline let $a" entry for the indirect query.
- inline_doesNotFireWhen_letReferencedTwice -- gate (count != 1).
- inline_doesNotFireWhen_letBoundToCount -- gate (body is not a
FilteredExpression).
- inline_doesNotFireWhen_letIsTyped -- gate (sequenceType present).
- issue873_indirectQueryUnderLoosePerfBound -- a 20x ceiling +
500ms slack, loose enough to avoid CI flakiness while still
catching the original 167x regression.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Within-fork PR for review. Stacked on
optimizer/expression-optimize-method(the four foundationalExpression.optimize(CompileContext)framework commits). Will be re-targeted ateXist-db/exist:developonce that framework lands upstream.Summary
Closes eXist-db#873.
A
let $v := <persistent path> return $v[Optimizable-pred]query was running ~167x slower than the equivalent direct form. This PR adds a narrow rewrite at theLetExpr.optimize(cc)stage that converts the indirect form into the direct form when six soundness gates hold, so the legacy Optimizer pass attaches the(#exist:optimize#)pragma to the resulting LocationStep and routes through the lucene/range pre-select.Diagnosis
For
Optimizer.visitFilteredExprdoes wrap the FilteredExpression in the optimize pragma. At runtimeOptimize.evalruns the lucene pre-select and obtains a smallresultNodeSet. But because the FE source is aVariableReferencerather than aLocationStep,before()recordscontextStep == null(BasicExpressionVisitor.findFirstStepfinds no LocationStep at the top level). WithcontextStep == null,Optimize.evalfalls through toinnerExpr.eval(result, null)-- butFilteredExpression.evalthen callsexpression.eval(contextSequence, ...)whereexpressionis the VariableReference, andVariableReference.evalreads the bound value off the local variable stack and ignorescontextSequence. Soseqbecomes the full//LINENodeSet andft:queryruns once per LINE; the pre-selectedresultis computed and discarded.What changed
exist-core/src/main/java/org/exist/xquery/LetInliner.javatryInline(LetExpr, CompileContext)checks the six gates; on success, attaches the FE's predicate to the input path's last LocationStep and returns the input path as the LetExpr's replacement (logged viacc.replaceWith).exist-core/src/main/java/org/exist/xquery/LetExpr.javaisScoreBinding()accessor; callsLetInliner.tryInline(this, cc)after the existing literal-drop gate.extensions/indexes/lucene/src/test/java/org/exist/indexing/lucene/LetInliningRegressionTest.javaThe six gates (
LetInliner.tryInline)getPreviousClause() == nulland the body is not aFLWORClause.Optimizer.visitFilteredExpr'sinstanceof LocationStepsimplification expects.The "inlining is not always desirable, in particular if there's no index defined" caveat noted in a 2016 comment on the issue is exactly gate 6: we only inline when the inlined form would expose an Optimizable to
visitLocationStep.Test plan
mvn test -pl extensions/indexes/lucene-- newLetInliningRegressionTestpasses (6/6); existingOptimizerTest,LuceneIndexTestpass; 5 pre-existingft-facetsfailures unchanged (verified by stash-and-rerun).mvn test -pl extensions/indexes/range-- 425/425 pass + 3 skipped, no new failures.mvn test -pl exist-core-- baseline 34 fail / 20 err vs with-fix 35 fail / 20 err (+1 = known flaky 503 HTTP test); unique failure set is byte-identical.LetIndirectionBenchmark-- depends on theexist-indexes-jmhmodule on a separate branch; will land as a follow-up.