Skip to content

Fix LIMIT in HASH_JOIN with recursive extend - #813

Merged
adsharma merged 9 commits into
LadybugDB:mainfrom
mdbenito:fix/limit-hash-join
Aug 18, 2026
Merged

Fix LIMIT in HASH_JOIN with recursive extend#813
adsharma merged 9 commits into
LadybugDB:mainfrom
mdbenito:fix/limit-hash-join

Conversation

@mdbenito

@mdbenito mdbenito commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

LIMIT pushdown can currently cross a query hash join above recursive extend without proving that rows can survive the join. A LIMIT 5 might stop recursive expansion after five candidate rows, but if a hash join or filter rejects one of them, the query will return only four rows even though more matching rows exist.

This PR pushes down a finite, overflow-safe SKIP + LIMIT only through the direct one-to-one inner rejoin generated for recursive extend. Intervening filters and all other hash joins block this pushdown, allowing recursion to continue until enough rows reach the outer limit. SKIP without LIMIT does not create a recursive cap.

I added several regression tests to cover the original join failure, filtered paths, and SKIP queries.


Update: Another test for a DISTINCT + SKIP case:

MATCH (a:person)
WITH DISTINCT a.age AS age
SKIP 2
RETURN ...;

Tinysnb contains seven distinct ages, so the correct result contains five rows. However, the optimizer was doing this:

  1. Read SKIP 2.
  2. Leave limitNumber equal to INVALID_LIMIT.
  3. Annotate LogicalDistinct with the skip value anyway.
  4. Let mapDistinct calculate a physical cap from the available fields.

mapDistinct started its calculation at zero:
cap = 0
no finite LIMIT, so add nothing
SKIP is 2, so cap = 2

The physical DISTINCT stopped after collecting two distinct values. The real outer SKIP then discarded those two values:

DISTINCT produces 2 rows + SKIP discards 2 rows => result contains 0 rows 💥

@adsharma
adsharma force-pushed the fix/limit-hash-join branch from 2b043a8 to 0caaf0e Compare August 17, 2026 01:07
@adsharma

Copy link
Copy Markdown
Contributor

The design question: can we avoid canPushLimitToHashJoin?

Yes — and you should, because the bool isn't encoding new information, it's re-deriving a structural fact that each operator already knows about itself.

The invariant the flag encodes is: "a finite outer cap is only valid on an operator beneath a chain of count-preserving operators; any operator that can discard rows between the LIMIT and the target invalidates the cap." That is a local property of each operator's type, not inherited context. So you can decide it at the point of traversal rather than threading it.

Recommendation — the clean, general, non-bool version of the same change: make FILTER a terminating barrier exactly like a non-rejoin HASH_JOIN already is, i.e. return without descending into its child, and delete the parameter entirely.

Looks like the tests pass after 0caaf0e. So the proposal is to drop canPushLimitToHashJoin and treatFilterAsBarrier and reduce this to a simple "do not optimize limit when filter is involved". It could be generalized into any operator that could potentially vary the count.

@mdbenito

Copy link
Copy Markdown
Contributor Author

I see... IIUC the clankers are saying (mine agrees) that a FILTER invalidates every cap this optimizer can currently push, so traversal should stop right there, and there's no need to carry a flag over.

I think (?) that makes sense, so I'll remove both my constructor flag and your method, and make FILTER an unconditional barrier.

As to the other operators, I think it would be best to handle those separately. I'm already stepping a bit outside my comfy zone with this PR.

@adsharma

Copy link
Copy Markdown
Contributor

that a FILTER invalidates every cap this optimizer can currently push

Exactly. I'm imagining it to be a big switch statement where we evaluate every operator, not just FILTER on "does this operator change the number of rows"?

The alternative: stateful visitor pattern could be hard to maintain and develop many corner cases over time. Some of it is inevitable (like using table stats to figure out if we want to push it down or not), but I want to make that call on a case-by-case basis by evaluating cost vs benefit.

Commit 2bbec9a is an example of this (see isPushDownSupported() in order_by_push_down_optimizer.cpp).

@mdbenito

Copy link
Copy Markdown
Contributor Author

that a FILTER invalidates every cap this optimizer can currently push

Exactly. I'm imagining it to be a big switch statement where we evaluate every operator, not just FILTER on "does this operator change the number of rows"?

I see. I'm afraid I don't have the bandwidth for this right now. What I can do is to simplify the change as I mentioned, I hope that's still useful.

I have also added another test for a DISTINCT + SKIP case which has led me to muddy the waters a little bit with another fix (see updated PR description).

mdbenito and others added 9 commits August 17, 2026 18:08
Only if all the following hold:

 1. A finite LIMIT exists. SKIP without LIMIT is not pushed.
 2. No FILTER has been crossed between the LIMIT and the HASH_JOIN.
 3. The hash join is an INNER join.
 4. LogicalHashJoin::requireFlatProbeKeys() returns false (the join cannot multiply probe-side rows)
 5. The probe-side shape is exactly PATH_PROPERTY_PROBE -> RECURSIVE_EXTEND.
 6. skipNumber + limitNumber cannot overflow common::INVALID_LIMIT.
…ag (additive)

Add a treatFilterAsBarrier toggle so the FILTER-terminated push-down can be
turned on/off without removing code. Default (true) keeps the new barrier
behavior the PR tests rely on; passing false restores the prior descent-
through-FILTER behavior. The original constructor, FILTER comment, and legacy
descent path are preserved verbatim; only the internal UNION_ALL construction
gains the flag so sub-branch optimizers honor the same mode.
…wnSupported

Factor the row-count-preserving pass-through operators (MULTIPLICITY_REDUCER,
EXPLAIN, ACCUMULATE, PROJECTION) into a single isPushDownSupported() predicate,
mirroring order_by_push_down_optimizer.cpp. FILTER and every other operator that
can discard rows now become barriers by default instead of needing per-case
exceptions.

Also guard TABLE_FUNCTION_CALL against SKIP-only caps and skip+limit overflow,
the same bug class previously fixed for DISTINCT and the recursive-extend join.
@adsharma
adsharma force-pushed the fix/limit-hash-join branch from 5da7f34 to 267cbc6 Compare August 18, 2026 01:09
@adsharma
adsharma merged commit 079aa84 into LadybugDB:main Aug 18, 2026
6 of 12 checks passed
@adsharma

Copy link
Copy Markdown
Contributor

Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants