Skip to content

Follow-ups from #6307: two of #6297's items lost their tracker, IN walks a range that answers in O(1), and EXPLAIN cannot describe the plan it would run #6323

Description

@lvca

Three follow-ups from #6307 (the #6297 / #6280 / #6279 / #6270 batch), filed together because that is where all three were found. Item 1 is a tracking regression created by that merge and is the reason this issue exists at all; items 2 and 3 are independent defects in the openCypher engine that the work happened to walk into.

1. Two of #6297's own items lost their tracker when #6297 was closed

#6297 carried four items. #6307 implemented items 1 and 2 and deliberately left items 3 and 4 alone, saying so in the PR description - but the commit's Closes #6297 trailer closed the issue anyway, and with it the only searchable record of the two that were not done:

Both now survive only as a comment in BaseRaftHATest (RESYNC_RETRY_TIMEOUT_MS at line 73, SLOW_WAIT_REPORT_MS at line 80) - which is exactly the condition #6297 was filed to escape, in its own words: "recorded in BaseRaftHATest's comment; this is just the tracking entry so it does not depend on someone rereading that file."

A search of open issues for RESYNC_RETRY_TIMEOUT_MS, reuseForks and the fork-isolation wording returns nothing, so this issue is now their tracker.

Worth folding in: two CI lanes are sized at the 60-minute job cap

Observed across the #6307 runs, and relevant to the fork-isolation item above because it is the same budget under pressure:

  • slow-unit-tests was cancelled at 1h0m16s in one run while progressing normally - no test failure, LSMTreeIndexTest had just completed - and passed in 46m on the most recent main run, having taken 57m40s and 39m27s on two earlier ones.
  • ha-integration-tests failed at exactly 1h0m8s, with 238 tests run and 1 error.

A lane whose honest duration sits within a few minutes of its own cap goes red on runner speed alone, and the failure reads as "the job failed" rather than "the job ran out of time", which is what made both look like regressions from the PR under test until the logs were pulled.

2. IN walks a range that can answer membership in O(1)

Found while diagnosing a CI failure in #6307: hugeRangeIsLazyWhenTheLimitIsDisabled measured 20,570 ms with 0 ms of JVM stall - not a pause, twenty seconds of real work - for three queries over range(0, 999999999). Timed individually:

   8 ms  RETURN size(range(0, 999999999)) AS r
   1 ms  RETURN range(0, 999999999)[123456789] AS r
3066 ms  RETURN 999999998 IN range(0, 999999999) AS r
   0 ms  RETURN 5 IN range(0, 999999999) AS r

size() and indexing are answered from start, step and size alone, as GHSA-xmjm's lazy LongRangeList intends. IN is not: InExpression.evaluateTernary (engine/src/main/java/com/arcadedb/query/opencypher/ast/InExpression.java, the loop at ~line 105) walks the list element by element, so the cost is the position of the match - index 5 returns instantly, index 999,999,998 costs three seconds locally and about twenty on a CI runner.

The walk is not gratuitous, which is why this needs care rather than a one-line change. It exists for Cypher's three-valued logic: a match returns early, but a non-match has to know whether any element compared null-uncertain (foundNull) to decide between null and false, so a miss walks the whole list unconditionally. And the per-element comparison is valuesCompare, which delegates to the = operator's comparator for the temporal, array and map coercion and the NaN contract that #5293 established - semantics List.contains does not have.

So the opportunity is narrower than "call contains()": it is a typed fast path, not a general one.

  • LongRangeList.indexOf is documented and implemented as O(1) (an exact-multiple-of-step test, engine/src/main/java/com/arcadedb/utility/LongRangeList.java:106-119), and a range of longs can never contain null - so foundNull is statically false and both the match and the miss are answerable without walking.
  • But indexOf deliberately rejects Double/Float (return -1 at line 116-117), while Cypher's = coerces numerically. 5.0 IN range(0, 10) would therefore flip from true to false if the fast path were taken unconditionally. Any fix has to gate on the left operand's type, not only on the right operand's.

Worth fixing on its own terms - a user writing x IN range(...) against a large range has no way to know the cost depends on where the value sits - and it is currently costing the unit lane about 26 s in CypherRangeHeapExhaustionTest alone.

3. EXPLAIN cannot describe the plan it would run, for any query on the traditional path

CypherExecutionPlan.explain() (engine/src/main/java/com/arcadedb/query/opencypher/executor/CypherExecutionPlan.java:702) describes two things: the count push-down chain, and the cost-based physical plan. A query the optimizer does not claim gets neither, and falls to the branch at line 729:

EXPLAIN MATCH (a),(b) WHERE ID(a) = $sourceId AND ID(b) = $targetId RETURN a, b

OpenCypher Native Execution Plan
=================================

Using Traditional Execution (Non-Optimized)

Reason: Query pattern not yet supported by optimizer
Execution will use step-by-step interpretation

That is the whole answer - no steps, no order, nothing about what the query will actually do. profile() does not have this gap: its else branch at line 850 appends the real step chain, and PROFILE of the same query returns

Execution Plan (Traditional):
+ MATCH NODE (a) [id: #1:0]
+ MATCH NODE (b) [id: #4:0]
+ FILTER WHERE (ID(a) = $sourceId AND ID(b) = $targetId)
+ PROJECT RETURN a, b
+ FINAL PROJECTION [a, b]

so the information exists and is already formatted - appendStepChain is shared - it is simply not reached from explain().

The consequence is that EXPLAIN, the one command whose entire purpose is inspecting a plan without running it, is strictly less informative than PROFILE for exactly the queries a user is most likely to be investigating. The workaround is to run PROFILE, which executes: not an option for a slow query, and actively wrong for a writing one, since PROFILE bypasses the idempotency gate and a PROFILE MATCH ... SET ... does write.

It also undercuts the [id: ...] marker #6307 just added to MatchNodeStep.prettyPrint. That marker exists so a user can confirm the RID push-down fired, which is what #3216's resolution asks them to arrange - and on the traditional path, which is where those queries land, EXPLAIN will not show it. #6307's own regression test had to use PROFILE for this reason.

Suspected fix: build the step chain in explain() the way profile() already does and append it, rather than printing a reason and stopping. The steps are constructed without being pulled, so nothing executes.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions