fix(#6323): membership in a range is a division, and EXPLAIN describes the plan it would run - #6344
Conversation
…s the plan it would run Three follow-ups from #6307, plus two defects the work walked into. 1. The two items #6297 carried and #6307 did not implement lost their tracker when the `Closes #6297` trailer closed the issue. They are refiled as #6343, and the comments in `BaseRaftHATest` that were the only remaining record now name it. The `slow-unit-tests` lane was capped at 60 min while its honest duration has been measured between 39m27s and 57m40s, and one run was CANCELLED at 1h0m16s while progressing normally: a cap inside its own spread goes red on runner speed alone, and it goes red as "the job failed". It is a hang detector, so it is now sized above the slowest honest run, at 90 min. 2. `IN` walked the list element by element, so against the lazy `range()` of GHSA-xmjm its cost was the POSITION of the match - 3 s locally and about 20 s on a CI runner for an element near the end of `range(0, 999999999)`, and the full walk for every miss. A range is an arithmetic progression whose elements are never null, so both the match and the miss are a division. The walk is not replaced by `List.contains`: membership is equality, and Cypher's `=` coerces numerically (`5.0 = 5` is true, as in Neo4j) where `equals()` does not, so the fast path reproduces the comparator's own answer - the RID-string interop included - and declines to the walk past 2^53, where a double stops naming one long. `null IN list` is answered from the list's emptiness alone, for any list. `LongRangeList.indexOf` keeps answering the `List` contract, which is `equals()`, and now rejects `BigInteger`/`BigDecimal` as it already rejected `Double`/`Float`: truncating them to a long made every value congruent to an element modulo 2^64 answer as that element. Membership by value is `containsLong`, for the callers whose equality coerces. 3. `EXPLAIN` described only the count push-down and the cost-based physical plan, so a query the optimizer does not claim got a reason and nothing else, while `PROFILE` of the same query printed the whole step chain. That left the one command whose purpose is inspecting a plan WITHOUT running it strictly less informative than the one that runs it - no use for a slow query, and wrong for a writing one, since `PROFILE MATCH ... SET ...` does write. The chain is now built and never pulled, and described; a UNION is described branch by branch, and the structured step list is published alongside the text. Found while doing the above: - Slicing a range copied the slice into an `ArrayList`, so `range(0, 999999999)[0..1000000000]` allocated a billion boxed longs and exhausted the heap - the failure the lazy range was introduced to remove. A slice of a range is a range. The slicing itself moves into the AST node, which `ExpressionEvaluator` now calls instead of carrying a second copy of it that was fixed one at a time. - Nine steps closed the parenthesis of their profiling suffix before the row count joined it, printing `(511us), 1 rows)`. Comparing the EXPLAIN and PROFILE chains is what made the two texts comparable enough to notice.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 4 |
🟢 Coverage 81.89% diff coverage · -6.57% coverage variation
Metric Results Coverage variation ✅ -6.57% coverage variation Diff coverage ✅ 81.89% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (34482fd) 160387 127383 79.42% Head commit (d94b612) 192808 (+32421) 140474 (+13091) 72.86% (-6.57%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#6344) 127 104 81.89% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
ReviewWent through the diff in detail, cross-checking the
|
…y which label the edge count filters on Review follow-ups on PR #6344. The suggested smoke test - that SET, REMOVE, DELETE, MERGE, FOREACH and a relationship CREATE are described by EXPLAIN without firing, not only CREATE - found that EXPLAIN of a write the optimizer claims described the scan and never mentioned the write at all. The optimizer claims the MATCH pattern only; every clause after it stays an execution step appended to the operator chain, and printing the physical plan alone left them out. Both EXPLAIN and PROFILE now name them, under the physical plan they run after. That in turn made `CountEdgesReturnStep` visible in a plan where a test asserted it was absent. The assertion was stale: the step has taken a targetLabel since the push-down learned to filter by it, so it applies to a labelled counted node and counts what that label reaches - which the test's own correctness assertion has been proving all along. It held only because the plan text never printed the step. The step now prints the label it filters on, for the reason MatchNodeStep prints the id it seeks, and the test asserts what actually happens. Also from the review: the Short/Byte case in rangeMembership says why converting through a long and comparing through a double reach the same answer; the optimized branch of explain() says that its structured steps are built from the same PhysicalPlan instance the text is printed from, so the two cannot describe different plans, and the test asserts they name the same operator; the LogManager import moves into alphabetical position.
ReviewWent through the full diff (workflow timeout, One thing worth a look:
before Everything else — the CI timeout bump (backed by concrete measured run durations), the |
…d of claiming the optimizer was not used Review follow-up on PR #6344. A UNION has no plan of its own: the planner leaves its physicalPlan null and plans each branch separately, so `canUseOptimizedPhysicalPlan()` is always false for the union itself and EXPLAIN printed "Reason: Query pattern not yet supported by optimizer" for every UNION - directly above the `+ OPTIMIZED MATCH (physical operators)` that a branch the optimizer DID claim prints underneath it. The text now says what is actually true, and the test asserts both halves: the per-branch header, and that each branch reports the path it takes.
ReviewI went through this PR in depth: the CI timeout fix, the Correctness - verified against the real comparator
|
…astException on one of them Review follow-up on PR #6344. The slice dedup left the bound parsing behind: the AST node validated a non-numeric bound and said what was wrong with the query, while the aggregation-aware evaluator cast straight to Number and raised a bare ClassCastException for the same query. That is the divergence the refactor exists to remove, so the bound parsing is shared too.
ReviewWent through the diff in detail, cross-checked the new 1. 2. } catch (final Exception e) {
LogManager.instance().log(this, Level.FINE, "Error on building the execution plan to describe it", e);
return null;
}This is documented and defensible (a bad plan should degrade to "no steps shown" rather than fail Strengths worth calling out:
No functional issues found in the core logic. The two points above are polish, not blockers. |
…e 2^53 bound says why it is inclusive Review follow-ups on PR #6344. The suggestion to relax the exact-double bound from `>= 2^53` to `> 2^53`, to match DistinctNumericKey, would have introduced a wrong answer: 2^53 is exactly representable, but 2^53+1 is NOT, and rounds ties-to-even down onto the same double, so at that magnitude two longs already share one double. Taking the fast path there would pick one of them and answer false for a range that holds only the other. The bound stays inclusive, the comment now says why, and a test builds both ranges and pins each answer against the walk's. EXPLAIN no longer swallows a plan it could not build into a FINE log nobody has enabled. The failure is named in the description instead: EXPLAIN answers a question about a query, and a query that cannot be planned is an answer to it - the statement would fail the same way when run. A query that does not PARSE keeps raising, since there is no plan to describe and the parse error already says why.
ReviewI read through the full diff (workflow timeout, Correctness (verified, no issues found)
Minor / nit
Test coverageStrong - Performance & securityNo concerns. The range fast-path avoids boxing/copies as intended (verified against the actual comparator semantics rather than just Nice work - this is a solid fix with the reasoning for each non-obvious decision (2^53 inclusivity, why |
…is starting with a digit Review follow-up on PR #6344. The EXPLAIN/PROFILE chain comparison stripped from the first `(<digit>` to the end of the line, so a step whose own text ended in one would have dropped out of the comparison along with the timing. The pattern now matches the shape AbstractExecutionStep actually appends: a duration in microseconds, optionally followed by a row count.
ReviewWent through the full diff (all 6 commits) plus the current state of the touched files. This is a very well-executed PR: the reasoning is carefully laid out in the commit messages/comments, the fast path for A few minor/non-blocking observations:
Overall this looks solid and ready to merge from a correctness/testing standpoint; the two notes above are things I'd only ask about, not block on. |
…y its failure message is safe to print Review follow-ups on PR #6344, both non-blocking. A CALL is the one step whose body is a plan of its own, so it is the one that could do eager work while being built rather than while being pulled. Both shapes join the write-safety test: a CALL subquery that writes, and a procedure call. The message appendPlanBuildFailure prints tells the caller nothing that running the same statement would not - execute() builds this very chain and lets what it throws reach the client, and EXPLAIN needs no privilege beyond running the query it describes. Said in the javadoc, since the question is a fair one to ask of any text that reaches a user.
ReviewI read through the full diff (engine changes, the two new test classes, and the CI timeout change). Overall this is a well-scoped PR: each of the three fixes is backed by a targeted regression test, and the fast paths are explicitly cross-checked against the semantics of the general-case code they bypass rather than reimplementing them independently. I traced the key correctness claims by hand against the current source rather than taking the PR description at face value:
Minor/non-blocking observations:
CI timeout change — the Test coverage — thorough: the No correctness, security, or performance issues found. Nice work threading the needle between the fast path being a genuine optimization and not being a second, independently-maintained copy of the comparison/slicing semantics. |
…nouncing the route it would have taken Review follow-up on PR #6344, non-blocking. "Execution will use step-by-step interpretation" was a route-selection message, true when EXPLAIN reached that line without ever building anything. It is now the branch for a statement that built no chain at all, where announcing the route describes nothing. It says what is true of such a statement instead, and the one test that asserted the old sentence asks for the step chain that replaced it.
ReviewI read through the full diff (engine changes + new/updated tests + the CI timeout change) and traced the key claims against the actual code rather than just the PR description. Overall this is a well-scoped, well-tested PR: each of the three fixes is backed by a regression test that pins the specific defect, and the "found while doing the above" fixes (range-slice heap exhaustion, unbalanced profiling parens) are real, narrowly-targeted bugs. Correctness
Minor observations (not blocking)
Test coverageStrong: No functional issues found. Nice, disciplined use of |
One conflict: #6348 deleted EagerDistinctCollectOptionalMatchStep, which this branch had touched for the unbalanced profiling parenthesis. The deletion wins - nothing on main references the class any more, and the fix dies with the file. The eight remaining paren fixes are unaffected, and no step added by #6348 carries the pattern.
|
Reviewed the full diff (all 8 commits) plus the current state of the touched files. This is a very well-constructed PR: TDD throughout, extensive drift-guard tests ( Code quality / correctness
Performance
Test coverage
Minor / nit
Overall: solid, well-tested change with unusually thorough self-review already baked into the commit history. Nothing here blocks merging. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6344 +/- ##
==========================================
+ Coverage 70.69% 70.73% +0.04%
==========================================
Files 1850 1850
Lines 160387 160443 +56
Branches 33713 33722 +9
==========================================
+ Hits 113384 113491 +107
+ Misses 33045 32999 -46
+ Partials 13958 13953 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Closes #6323.
Three follow-ups from #6307, and two defects the work walked into.
1. The two items #6297 carried that #6307 did not implement
They are refiled as #6343, which is searchable for
RESYNC_RETRY_TIMEOUT_MSand forreuseForks, and the comments inBaseRaftHATestthat were the only remaining record of them now name it. Neither can be decided inside a PR: the next cut of the budget needsSLOW WAITlines from runs at the new 5 s threshold, and the fork isolation needs a measurement of whatreuseForks=falsecosts this lane before it is imposed on it. #6343 says what evidence each one is waiting for.The CI cap is fixed here.
slow-unit-testswas capped at 60 min while its honest duration has been measured at 39m27s, 46m and 57m40s onmain, and one run was CANCELLED at 1h0m16s while progressing normally, with no test failed. A cap inside its own observed spread goes red on runner speed alone, and it goes red as "the job failed" rather than "the job ran out of time" - which is what made it read as a regression from the PR under test. The cap is a hang detector, so it is now sized above the slowest honest run: 90 min.ha-integration-testsis not capped at 60: it already hastimeout-minutes: 90, so its 1h0m8s red was the 1 error it reported, not the cap. Nothing to change there.2.
INwalked a range that answers in O(1)The reported cost, timed on this branch's parent:
The cost was the position of the match, and a miss walked all of it. A range is an arithmetic progression whose elements are never null, so both the match and the miss are a division - the three-valued logic that forces the walk on a general list has nothing to discover in a range.
The walk is not replaced by
List.contains. Membership is equality, and Cypher's=coerces numerically -5.0 IN range(0, 10)is true, as it is in Neo4j - whereequals()does not, which is exactly whyLongRangeList.indexOfrejectsDouble/Float. So the fast path reproduces the comparator's own answer rather than calling a different one:=compares them;Numbergoes throughdoubleValue(), as=does: NaN and the infinities match nothing, a fractional value matches nothing, and past 2^53 - where a double stops naming a single long - it declines and the walk runs, because there the walk is the definition of the answer;id()interop coercion=applies, not a second one;Long, and for=that is simply not equal.null IN <list>is now answered from the list's emptiness alone, for any list: every comparison against null is null, so only the number of elements matters. It used to walk a billion elements to conclude that.The drift guard is
answersExactlyAsTheWalkDoes: 30 operands covering each of those branches, each asserted equal to the same operand against a materialised list. A new coercion inComparisonExpressionthat the fast path does not learn turns that test red.LongRangeList.indexOfalso now rejectsBigInteger/BigDecimal, as it already rejectedDouble/Float. It answers theListcontract, which isequals(), and noLongequals aBigInteger; truncating them to a long made every value congruent to an element modulo 2^64 answer as that element. Membership by value is the newcontainsLong, for callers whose own equality coerces.3.
EXPLAINcould not describe the plan it would runEXPLAIN MATCH (a),(b) WHERE ID(a) = $sourceId AND ID(b) = $targetId RETURN a, bused to answer with a reason and stop. It now answers:The steps are built and never pulled, which is what makes describing a write a description - and is why the workaround of running
PROFILEnever was one:PROFILE MATCH ... SET ...writes.explainingAWriteDescribesItWithoutPerformingItpins that down. A UNION is answered branch by branch, so it is described branch by branch. The structured step list is published alongside the text, so a client reading the plan as data sees the same chain, andexplainAndProfileDescribeTheSameChainasserts the two texts agree once PROFILE's per-step timings are stripped - the timings being the only thing running the query buys.This also restores the
[id: ...]marker #6307 added forEXPLAIN: it exists so a user can confirm the RID push-down fired, and those queries land on exactly the path that had no plan to show.Found while doing the above
range(0, 999999999)[0..1000000000]allocated a billion boxed longs and exhausted the heap - the failure the lazy range was introduced to remove (GHSA-xmjm-8q85-g778). A slice of an arithmetic progression is one, andLongRangeList.subListalready returns it in constant space. The slicing itself moved into the AST node, whichExpressionEvaluatornow calls instead of carrying a second copy of it: there were two, and only one of them was being fixed.(511us), 1 rows), closing the parenthesis before the row count joined the same group. Comparing the EXPLAIN and PROFILE chains is what made the two texts comparable enough for it to show.Verification
CypherInRangeMembershipTest(7 tests),CypherExplainTraditionalPlanTest(8 tests).CypherRangeHeapExhaustionTest.hugeRangeIsLazyWhenTheLimitIsDisabled-INmoves inside the stall-discounted measured window it was excluded from in The server and network suites still bound wall clock the way the engine suite did before #6260 #6270, since it is now constant-cost;LongRangeListTestgains theindexOfcontract andcontainsLongcases.mvn test -pl engine -Dtest='com.arcadedb.query.opencypher.**': 8202 tests, 0 failures, 0 errors.PostCommandHandlerProfileIT(server): 9 tests green - the HTTPexplainPlanfield now carries steps for EXPLAIN too.test-compilegreen.What the review rounds added
Seven review passes; the last one found no correctness, security or performance issues. What they changed:
EXPLAINof a write the optimizer claims described the scan and never mentioned the write. The optimizer claims the MATCH pattern only - RETURN, ORDER BY and every write clause stay execution steps appended to the operator chain - and printing the physical plan alone left them out. EXPLAIN and PROFILE now name them, under the plan they run after. Found by writing the review's suggested smoke test forSET/REMOVE/DELETE/MERGE/FOREACH/edge-CREATE/CALL.CountEdgesReturnStepvisible in a plan where a test asserted it was absent. The assertion was stale: the step has taken atargetLabelsince the push-down learned to filter by it, so it applies to a labelled counted node and counts what that label reaches - which the test's own correctness assertion had been proving all along. It held only because the plan text never printed the step. The step now prints[target: <label>], for the reasonMatchNodeStepprints[id: ...].EXPLAINof a UNION claimed "not yet supported by optimizer" while showing an optimized branch underneath it. A UNION has no plan of its own; each branch is planned separately. It now says so.ClassCastExceptionon the other - the exact divergence the dedup exists to remove.>= 2^53to> 2^53to matchDistinctNumericKey. It would be wrong: 2^53 is representable but 2^53+1 is not, and rounds ties-to-even onto the same double, so two longs already share it there. The fast path would pick one and answerfalsefor a range holding only the other. The bound stays inclusive, with a test that builds both ranges.