Yo DAWGS, I heard you like rewrites#70
Open
zinic wants to merge 1 commit intoSpecterOps:mainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds lateral-subquery AST/formatting, seed-driven recursive expansion with lateral lookup joins, bidirectional/pair-aware shortest-path decision logic and harness parameterization (filters + path_limit), limit-pushdown into harness calls, many regenerated SQL translation fixtures, and accompanying unit/integration tests and migrations. ChangesBidirectional shortest-path, seed-driven expansion & harness parameterization
Sequence DiagramsequenceDiagram
participant Client
participant Translator
participant CapCheck as CapabilityChecker
participant ExpBuilder as ExpansionBuilder
participant DB as DBHarness
Client->>Translator: Translate request (traversal/shortest-path)
Translator->>CapCheck: Evaluate bidirectional/selective/pair-aware eligibility
CapCheck-->>Translator: Decision (UseBidirectionalSearch true/false)
alt UseBidirectionalSearch = true
Translator->>ExpBuilder: Build seed CTEs, lateral lookups, harness params (filters, path_limit)
ExpBuilder-->>Translator: CTEs + bidirectional harness call
Translator->>DB: Call bidirectional_*_harness(filter/array, path_limit)
DB->>DB: Populate traversal_filter tables, expand fronts, resolve pairs, emit paths
DB-->>Translator: Paths result
else
Translator->>ExpBuilder: Build primer/recursive expansion with seed CTEs
ExpBuilder-->>Translator: CTEs + unidirectional harness call
Translator->>DB: Call unidirectional_*_harness(array/filter, path_limit)
DB->>DB: Expand frontier, prune visited, emit paths
DB-->>Translator: Paths result
end
Translator-->>Client: Final result set
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cypher/models/pgsql/translate/expansion.go (1)
722-723: 💤 Low valueConsider checking the error from type conversion.
The error from
pgsql.As[pgsql.SelectItem]is being ignored. While the conversion should always succeed for expression types, silently discarding errors can mask issues if the API changes.♻️ Suggested fix
- satisfiedSelectItem, _ := pgsql.As[pgsql.SelectItem](satisfied) - return satisfiedSelectItem + satisfiedSelectItem, err := pgsql.As[pgsql.SelectItem](satisfied) + if err != nil { + // This should never happen for expression types + return satisfied + } + return satisfiedSelectItemThe same pattern appears at line 765-766 in
backwardTerminalSatisfaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/translate/expansion.go` around lines 722 - 723, The code ignores the error returned by pgsql.As[pgsql.SelectItem] when converting satisfied (satisfiedSelectItem, _ := pgsql.As[pgsql.SelectItem](satisfied)); capture the second return value (err) and handle it instead of discarding it: check err after calling pgsql.As[pgsql.SelectItem](satisfied) and return or propagate a descriptive error from the enclosing function (or fail fast) depending on the function's error handling pattern. Apply the same change inside backwardTerminalSatisfaction where pgsql.As[pgsql.SelectItem] is called so both satisfiedSelectItem conversions are checked and errors are handled consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cypher/models/pgsql/translate/model.go`:
- Around line 298-313: The method CanExecutePairAwareBidirectionalSearch
currently returns true for pair-aware bidirectional search whenever expansion
and endpoint constraints exist; update it to measure endpoint selectivity and
ensure at least one endpoint qualifies as a bidirectional anchor before
returning true: call endpointSelectivity(...) for the left and right endpoints
(using s.Expansion and s.LeftNode.Identifier / s.RightNode.Identifier), check
for and propagate any error, then call isBidirectionalSearchAnchor(...) on each
selectivity and only return true if at least one endpoint is an anchor;
otherwise return false. Ensure you reference and use the existing functions
endpointSelectivity and isBidirectionalSearchAnchor and handle their
error/return values instead of unconditionally returning true.
In `@drivers/pg/query/sql/schema_down.sql`:
- Around line 19-50: The rollback script is missing drops for functions added in
schema_up; update drivers/pg/query/sql/schema_down.sql to mirror schema_up by
adding DROP FUNCTION IF EXISTS entries for the new overloads and helper builders
— specifically add drops for the unidirectional_sp_harness overloads that
include the path_limit parameter (the new signatures of
unidirectional_sp_harness(..., path_limit)), the unidirectional_asp/sp helper
overloads if any added, and the helper table-creation functions
create_unidirectional_shortest_path_tables() and
create_bidirectional_pathspace_tables() so that
create_unidirectional_shortest_path_tables,
create_bidirectional_pathspace_tables, and the new
unidirectional_sp_harness(...) signatures are removed on downgrade; ensure the
signatures in the DROP FUNCTION lines exactly match the signatures introduced in
schema_up for functions like unidirectional_sp_harness and the two
create_*_tables helpers.
In `@drivers/pg/query/sql/schema_up.sql`:
- Around line 1771-1779: The three early-return branches in
bidirectional_sp_harness (the forward-hit, backward-hit and midpoint-match exits
that currently "return query" from forward_front/backward_front/midpoint
results) ignore path_limit; modify each of those return paths to honor the
path_limit guard used elsewhere (apply the same conditional that truncates
results when path_limit is set — e.g., add the path_limit-based WHERE/ROW_NUMBER
or LIMIT predicate used in the other exit) so those branches cannot return more
rows than path_limit; update the return query for the forward_front (alias r),
backward_front and midpoint-match branches to include that same path_limit
check/pushdown logic (the same pattern applied at the other locations referenced
in the comment).
---
Nitpick comments:
In `@cypher/models/pgsql/translate/expansion.go`:
- Around line 722-723: The code ignores the error returned by
pgsql.As[pgsql.SelectItem] when converting satisfied (satisfiedSelectItem, _ :=
pgsql.As[pgsql.SelectItem](satisfied)); capture the second return value (err)
and handle it instead of discarding it: check err after calling
pgsql.As[pgsql.SelectItem](satisfied) and return or propagate a descriptive
error from the enclosing function (or fail fast) depending on the function's
error handling pattern. Apply the same change inside
backwardTerminalSatisfaction where pgsql.As[pgsql.SelectItem] is called so both
satisfiedSelectItem conversions are checked and errors are handled consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bf4d424-c97d-49a9-9d5b-9e1697489ecd
📒 Files selected for processing (18)
.gitignorecypher/models/pgsql/format/format.gocypher/models/pgsql/format/format_test.gocypher/models/pgsql/functions.gocypher/models/pgsql/model.gocypher/models/pgsql/test/translation_cases/multipart.sqlcypher/models/pgsql/test/translation_cases/pattern_binding.sqlcypher/models/pgsql/test/translation_cases/pattern_expansion.sqlcypher/models/pgsql/test/translation_cases/shortest_paths.sqlcypher/models/pgsql/test/validation_integration_test.gocypher/models/pgsql/translate/constraints_test.gocypher/models/pgsql/translate/expansion.gocypher/models/pgsql/translate/model.gocypher/models/pgsql/translate/pattern.gocypher/models/pgsql/translate/projection.gocypher/models/pgsql/translate/selectivity.godrivers/pg/query/sql/schema_down.sqldrivers/pg/query/sql/schema_up.sql
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypher/models/pgsql/translate/expansion.go (1)
769-840:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBound shortest-path seeds can generate out-of-scope predicates.
Line 779 tries to “deframe” bound root predicates before pushing them into the seed CTE, but that helper only rewrites a narrow subset of expression nodes; Line 930 does not even do that for bound terminal predicates. These seed queries only range over
traversal_root_filter/traversal_terminal_filterplusnode, so any bound endpoint constraint that still references the previous frame or another bound symbol will format invalid SQL. Reuse the same locality split used inbuildExpansionPatternRoot, and keep non-local terms out of the seed query unless you also join the previous frame into the seed.Also applies to: 922-986
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypher/models/pgsql/translate/expansion.go` around lines 769 - 840, The seed CTEs (created in prepareForwardFrontPrimerQuery via newExpansionRootIDsParameterSeed/newExpansionNodeSeed when traversalStep.LeftNodeBound is true and usesBoundRootIDs() is true) can receive predicates that reference out-of-scope/bound symbols; instead of calling deframeExpression() blindly, split primerNodeConstraints into local vs non-local terms using the same locality-split logic used by buildExpansionPatternRoot, pass only the local terms into the seed creation (rootIDs/node seed), and keep non-local terms out of the seed (or join the previous frame into the seed if you intentionally need them). Apply the same locality filtering to terminal predicates before adding them to any seed or join (the code paths controlled by expansionModel.TerminalNodeConstraints, UseMaterializedTerminalFilter and UseMaterializedEndpointPairFilter), and ensure any place that uses deframeExpression is replaced with this locality-aware split so seeds only contain predicates that reference symbols present in the seed CTE.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cypher/models/pgsql/translate/expansion.go`:
- Around line 769-840: The seed CTEs (created in prepareForwardFrontPrimerQuery
via newExpansionRootIDsParameterSeed/newExpansionNodeSeed when
traversalStep.LeftNodeBound is true and usesBoundRootIDs() is true) can receive
predicates that reference out-of-scope/bound symbols; instead of calling
deframeExpression() blindly, split primerNodeConstraints into local vs non-local
terms using the same locality-split logic used by buildExpansionPatternRoot,
pass only the local terms into the seed creation (rootIDs/node seed), and keep
non-local terms out of the seed (or join the previous frame into the seed if you
intentionally need them). Apply the same locality filtering to terminal
predicates before adding them to any seed or join (the code paths controlled by
expansionModel.TerminalNodeConstraints, UseMaterializedTerminalFilter and
UseMaterializedEndpointPairFilter), and ensure any place that uses
deframeExpression is replaced with this locality-aware split so seeds only
contain predicates that reference symbols present in the seed CTE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b02fbfc5-bccb-432b-ac66-9ab31466e0a2
📒 Files selected for processing (18)
.gitignorecypher/models/pgsql/format/format.gocypher/models/pgsql/format/format_test.gocypher/models/pgsql/functions.gocypher/models/pgsql/model.gocypher/models/pgsql/test/translation_cases/multipart.sqlcypher/models/pgsql/test/translation_cases/pattern_binding.sqlcypher/models/pgsql/test/translation_cases/pattern_expansion.sqlcypher/models/pgsql/test/translation_cases/shortest_paths.sqlcypher/models/pgsql/test/validation_integration_test.gocypher/models/pgsql/translate/constraints_test.gocypher/models/pgsql/translate/expansion.gocypher/models/pgsql/translate/model.gocypher/models/pgsql/translate/pattern.gocypher/models/pgsql/translate/projection.gocypher/models/pgsql/translate/selectivity.godrivers/pg/query/sql/schema_down.sqldrivers/pg/query/sql/schema_up.sql
✅ Files skipped from review due to trivial changes (3)
- cypher/models/pgsql/functions.go
- .gitignore
- cypher/models/pgsql/translate/selectivity.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cypher/models/pgsql/format/format_test.go
- cypher/models/pgsql/translate/pattern.go
- drivers/pg/query/sql/schema_down.sql
- cypher/models/pgsql/translate/constraints_test.go
- cypher/models/pgsql/test/validation_integration_test.go
- cypher/models/pgsql/test/translation_cases/shortest_paths.sql
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cypher/models/pgsql/translate/model.go (1)
313-323:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPair-aware anchoring still requires both endpoints to clear the threshold.
This branch now returns
falseas soon as the primer side is below threshold, and then still requires the terminal side to pass. In practice that means pair-aware bidirectional search only turns on when both sides are anchors, so one-sided-anchor shortest-path queries fall back to the slower unidirectional plan.Suggested fix
- if primerSelectivity, err := s.endpointSelectivity(scope, s.Expansion.PrimerNodeConstraints, s.LeftNodeBound); err != nil { - return false, err - } else if !isBidirectionalSearchAnchor(primerSelectivity) { - return false, nil - } - - if terminalSelectivity, err := s.endpointSelectivity(scope, s.Expansion.TerminalNodeConstraints, s.RightNodeBound); err != nil { - return false, err - } else { - return isBidirectionalSearchAnchor(terminalSelectivity), nil - } + primerSelectivity, err := s.endpointSelectivity(scope, s.Expansion.PrimerNodeConstraints, s.LeftNodeBound) + if err != nil { + return false, err + } + + terminalSelectivity, err := s.endpointSelectivity(scope, s.Expansion.TerminalNodeConstraints, s.RightNodeBound) + if err != nil { + return false, err + } + + return isBidirectionalSearchAnchor(primerSelectivity) || + isBidirectionalSearchAnchor(terminalSelectivity), nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypher/models/pgsql/translate/model.go` around lines 313 - 323, The current branch short-circuits on the primer side and prevents one-sided anchors from enabling pair-aware bidirectional search; instead, compute both endpoint selectivities (call endpointSelectivity for s.Expansion.PrimerNodeConstraints with s.LeftNodeBound and for s.Expansion.TerminalNodeConstraints with s.RightNodeBound), handle errors for each, and then return the OR of isBidirectionalSearchAnchor(primerSelectivity) || isBidirectionalSearchAnchor(terminalSelectivity) so that a single-side anchor can enable pair-aware search.
🧹 Nitpick comments (2)
cypher/models/pgsql/translate/references_test.go (2)
51-57: 💤 Low valueTest name slightly misrepresents the scenario
TestCollectReferencedIdentifiersIncludesRepeatedMatchPatternDeclarationsfocuses onbbeing present while silently encoding the equally important assertion thatais not present (Line 54). The name suggests the function includes things from repeated MATCHes broadly, whereas the actual invariant is that cross-clause reference liveness is tracked correctly — unreferenced pattern variables likeaare excluded. A name likeTestCollectReferencedIdentifiersTracksInterClauseReferenceLivenesswould better capture both sides.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypher/models/pgsql/translate/references_test.go` around lines 51 - 57, Rename the test function TestCollectReferencedIdentifiersIncludesRepeatedMatchPatternDeclarations to a name that reflects the liveness semantics (e.g., TestCollectReferencedIdentifiersTracksInterClauseReferenceLiveness) and update the function identifier accordingly so the test reads: referencedIdentifiers := referencedIdentifiersForQuery(...) and the same assertions remain; ensure any test registrations or references to the old function name are updated to avoid broken references.
13-25: 💤 Low value
referencedIdentifiersForQuerysilently skips multi-part queriesThe helper always accesses
regularQuery.SingleQuery.SinglePartQuerywithout asserting thatSinglePartQueryis non-nil. If a test accidentally passes a query that maps to a multi-part structure (e.g., containingWITH),collectReferencedIdentifierswould be called withnil, likely panicking or returning an opaque error rather than a clear test failure.🛡️ Proposed fix
regularQuery, err := frontend.ParseCypher(frontend.NewContext(), cypherQuery) require.NoError(t, err) require.NotNil(t, regularQuery.SingleQuery) require.NotNil(t, regularQuery.SingleQuery.SinglePartQuery) +require.Nil(t, regularQuery.SingleQuery.MultiPartQuery, "helper only handles single-part queries; use a dedicated helper for multi-part queries") referencedIdentifiers, err := collectReferencedIdentifiers(regularQuery.SingleQuery.SinglePartQuery)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypher/models/pgsql/translate/references_test.go` around lines 13 - 25, The helper referencedIdentifiersForQuery currently dereferences regularQuery.SingleQuery.SinglePartQuery without asserting it's present; add a require.NotNil(t, regularQuery.SingleQuery.SinglePartQuery, "expected single-part query (received multi-part/WITH) before calling collectReferencedIdentifiers") immediately after the existing require.NotNil for SingleQuery so collectReferencedIdentifiers is never called with nil; this makes the test fail fast and provides a clear message when a test accidentally supplies a multi-part query.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cypher/models/pgsql/test/translation_cases/multipart.sql`:
- Around line 38-39: The final query has a cartesian product issue because it
includes s1 and s3 in the FROM clause without correlating them on s3.root_id and
(s1.n0).id, causing duplicate paths and inflated counts in the count(c) or
adminCount. To fix it, either remove s1 from the final FROM clause or add an
explicit join condition correlating s1 and s3 on (s1.n0).id = s3.root_id within
the relevant subquery that produces s2, ensuring each recursive path corresponds
correctly to its seed row.
- Around line 92-93: The translation loses OPTIONAL MATCH semantics because s1
builds from inner joins (edge e0 join node n0 join node n1), so nodes with no
matching e0 are filtered out; change the inner query that defines s1 to start
from the candidate node n0 and use LEFT JOIN for e0 (and LEFT JOIN for n1
conditioned on e0.start_id) so n0 rows are preserved when no edge exists, then
keep the aggregation in s0 (count(e0) will yield 0 for nulls) so s0.i0 = 0
correctly selects nodes with zero relationships; locate and update the joins in
the s1 definition around e0, n0, n1 to implement this left-join seed.
In `@cypher/models/pgsql/test/translation_cases/quantifiers.sql`:
- Around line 41-45: Remove the duplicated quantifier fixtures: delete the two
SQL case blocks that begin with "-- case: MATCH (m:NodeKind1) WHERE
m.unconstraineddelegation = true ..." and "-- case: MATCH (m:NodeKind1) WHERE
ANY(name in m.serviceprincipalnames WHERE name CONTAINS "PHANTOM") ..." so only
the original occurrences remain; ensure you remove the entire WITH ... SELECT
... statement for each duplicate (the s0/s2 CTE blocks and their final SELECT)
so the fixture loader no longer sees byte-for-byte duplicate cases.
In `@cypher/models/pgsql/translate/projection.go`:
- Around line 573-599: The helper directUnidirectionalShortestPathHarnessFrame
only recognizes pgsql.FunctionUnidirectionalSPHarness so endpoint-alias
discovery fails for bidirectional CTEs; update this routine
(directUnidirectionalShortestPathHarnessFrame) to also detect
pgsql.FunctionBidirectionalSPHarness (or the bidirectional harness enum/name
used in the project) when scanning selectBody.From.function calls, treat it the
same as FunctionUnidirectionalSPHarness for alias discovery, and keep the
existing duplicate-harness check and return semantics so
shortestPathLimitPushdownTransparentWhere can correctly push limits for
bidirectional shortest-path harnesses as well.
---
Duplicate comments:
In `@cypher/models/pgsql/translate/model.go`:
- Around line 313-323: The current branch short-circuits on the primer side and
prevents one-sided anchors from enabling pair-aware bidirectional search;
instead, compute both endpoint selectivities (call endpointSelectivity for
s.Expansion.PrimerNodeConstraints with s.LeftNodeBound and for
s.Expansion.TerminalNodeConstraints with s.RightNodeBound), handle errors for
each, and then return the OR of isBidirectionalSearchAnchor(primerSelectivity)
|| isBidirectionalSearchAnchor(terminalSelectivity) so that a single-side anchor
can enable pair-aware search.
---
Nitpick comments:
In `@cypher/models/pgsql/translate/references_test.go`:
- Around line 51-57: Rename the test function
TestCollectReferencedIdentifiersIncludesRepeatedMatchPatternDeclarations to a
name that reflects the liveness semantics (e.g.,
TestCollectReferencedIdentifiersTracksInterClauseReferenceLiveness) and update
the function identifier accordingly so the test reads: referencedIdentifiers :=
referencedIdentifiersForQuery(...) and the same assertions remain; ensure any
test registrations or references to the old function name are updated to avoid
broken references.
- Around line 13-25: The helper referencedIdentifiersForQuery currently
dereferences regularQuery.SingleQuery.SinglePartQuery without asserting it's
present; add a require.NotNil(t, regularQuery.SingleQuery.SinglePartQuery,
"expected single-part query (received multi-part/WITH) before calling
collectReferencedIdentifiers") immediately after the existing require.NotNil for
SingleQuery so collectReferencedIdentifiers is never called with nil; this makes
the test fail fast and provides a clear message when a test accidentally
supplies a multi-part query.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e9d9113-8e78-40f4-a3b7-92f6e9740e6c
📒 Files selected for processing (28)
.gitignorecypher/models/pgsql/format/format.gocypher/models/pgsql/format/format_test.gocypher/models/pgsql/functions.gocypher/models/pgsql/model.gocypher/models/pgsql/test/translation_cases/delete.sqlcypher/models/pgsql/test/translation_cases/multipart.sqlcypher/models/pgsql/test/translation_cases/nodes.sqlcypher/models/pgsql/test/translation_cases/pattern_binding.sqlcypher/models/pgsql/test/translation_cases/pattern_expansion.sqlcypher/models/pgsql/test/translation_cases/quantifiers.sqlcypher/models/pgsql/test/translation_cases/shortest_paths.sqlcypher/models/pgsql/test/translation_cases/stepwise_traversal.sqlcypher/models/pgsql/test/translation_cases/update.sqlcypher/models/pgsql/test/validation_integration_test.gocypher/models/pgsql/translate/constraints_test.gocypher/models/pgsql/translate/expansion.gocypher/models/pgsql/translate/limit_pushdown_test.gocypher/models/pgsql/translate/model.gocypher/models/pgsql/translate/pattern.gocypher/models/pgsql/translate/projection.gocypher/models/pgsql/translate/query.gocypher/models/pgsql/translate/references.gocypher/models/pgsql/translate/references_test.gocypher/models/pgsql/translate/selectivity.gocypher/models/pgsql/translate/traversal.godrivers/pg/query/sql/schema_down.sqldrivers/pg/query/sql/schema_up.sql
✅ Files skipped from review due to trivial changes (3)
- cypher/models/pgsql/functions.go
- cypher/models/pgsql/translate/selectivity.go
- cypher/models/pgsql/format/format_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- .gitignore
- cypher/models/pgsql/translate/pattern.go
- cypher/models/pgsql/model.go
- cypher/models/pgsql/translate/constraints_test.go
- drivers/pg/query/sql/schema_down.sql
- cypher/models/pgsql/test/validation_integration_test.go
- cypher/models/pgsql/translate/expansion.go
- drivers/pg/query/sql/schema_up.sql
Contributor
Author
|
@CodeRabbit full review, please. |
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
zinic
force-pushed
the
main
branch
2 times, most recently
from
8461b20 to
2404e30
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cypher/models/pgsql/translate/expansion.go (1)
14-2172: ⚖️ Poor tradeoffLGTM — refactor is cohesive and aligns with the regenerated fixtures.
The new abstractions hang together well:
expansionSeed+newExpansionNodeSeed/newExpansionBoundNodeSeed/newExpansionNodeFilterSeedcleanly centralize the four primer-source shapes (free, bound-from-prior-frame, parameter-filter, parameter-filter-with-constraints), withMaterialized: falsematching thenot materializedseeds in the SQL fixtures.recursiveExpansionEdgeLookupJoin/expansionNodeLookupJoincorrectly emitJOIN LATERAL (... OFFSET 0) ... ON true, and theOFFSET 0rationale is documented inline.- The split between local and external satisfaction (
expansionTerminalSatisfactionLocality+DeferredNodeSatisfactionConstraint) keeps cross-frame predicates out of the recursive body and reapplies them at the projection — consistent with the newwhere s2.satisfiedplacement plus terminal predicates being applied post-aggregation inmultipart.sql.applyBoundEndpointProjectionConstraintscorrectly prepends the previous frame and ANDs the bound endpoint equalities only when at least one endpoint is bound, before the harness CTE join chain.shortestPathVisitedPruningConditionand the forward/backward visited-table selection (expansionVisitedvsexpansionForwardVisited/expansionBackwardVisited) cleanly gate the new bidirectional harness path, andtranslateShortestPathTraversalwiresUseBidirectionalSearchfromCanExecutePairAwareBidirectionalSearch, matching the dispatch incypher/models/pgsql/translate/pattern.go(snippets 1–2).A couple of small observations you may want to consider as follow-ups (non-blocking):
buildBiDirectionalShortestPathsHarnessCallonly setsUseMaterializedEndpointPairFilter; ifcanMaterializeEndpointPairFilterreturns false, bidirectional mode won't fall back toUseMaterializedTerminalFiltereven whencanMaterializeTerminalFilterwould allow it. That's a missed optimization rather than a correctness issue.prepareForwardFrontPrimerQuery/prepareBackwardFrontPrimerQueryandshortestPathsParameters/bidirectionalAllShortestPathsParametersshare substantial structure that could be parameterized by direction; deferrable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypher/models/pgsql/translate/expansion.go` around lines 14 - 2172, No code changes are required for correctness; the refactor is cohesive and LGTM, but as optional follow-ups: (1) consider adding a fallback in buildBiDirectionalShortestPathsHarnessCall to use canMaterializeTerminalFilter when canMaterializeEndpointPairFilter returns false (look at buildBiDirectionalShortestPathsHarnessCall, canMaterializeEndpointPairFilter, canMaterializeTerminalFilter, and where UseMaterializedEndpointPairFilter/UseMaterializedTerminalFilter are set), and (2) factor duplicated logic between prepareForwardFrontPrimerQuery/prepareBackwardFrontPrimerQuery and shortestPathsParameters/bidirectionalAllShortestPathsParameters into a shared helper to reduce repetition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cypher/models/pgsql/translate/expansion.go`:
- Around line 14-2172: No code changes are required for correctness; the
refactor is cohesive and LGTM, but as optional follow-ups: (1) consider adding a
fallback in buildBiDirectionalShortestPathsHarnessCall to use
canMaterializeTerminalFilter when canMaterializeEndpointPairFilter returns false
(look at buildBiDirectionalShortestPathsHarnessCall,
canMaterializeEndpointPairFilter, canMaterializeTerminalFilter, and where
UseMaterializedEndpointPairFilter/UseMaterializedTerminalFilter are set), and
(2) factor duplicated logic between
prepareForwardFrontPrimerQuery/prepareBackwardFrontPrimerQuery and
shortestPathsParameters/bidirectionalAllShortestPathsParameters into a shared
helper to reduce repetition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 483409c1-e69c-4c15-8d1a-3c4261b39c00
📒 Files selected for processing (29)
.gitignorecypher/models/pgsql/format/format.gocypher/models/pgsql/format/format_test.gocypher/models/pgsql/functions.gocypher/models/pgsql/model.gocypher/models/pgsql/test/translation_cases/delete.sqlcypher/models/pgsql/test/translation_cases/multipart.sqlcypher/models/pgsql/test/translation_cases/nodes.sqlcypher/models/pgsql/test/translation_cases/pattern_binding.sqlcypher/models/pgsql/test/translation_cases/pattern_expansion.sqlcypher/models/pgsql/test/translation_cases/quantifiers.sqlcypher/models/pgsql/test/translation_cases/shortest_paths.sqlcypher/models/pgsql/test/translation_cases/stepwise_traversal.sqlcypher/models/pgsql/test/translation_cases/update.sqlcypher/models/pgsql/test/validation_integration_test.gocypher/models/pgsql/translate/constraints_test.gocypher/models/pgsql/translate/expansion.gocypher/models/pgsql/translate/limit_pushdown_test.gocypher/models/pgsql/translate/model.gocypher/models/pgsql/translate/pattern.gocypher/models/pgsql/translate/projection.gocypher/models/pgsql/translate/query.gocypher/models/pgsql/translate/references.gocypher/models/pgsql/translate/references_test.gocypher/models/pgsql/translate/selectivity.gocypher/models/pgsql/translate/traversal.godrivers/pg/query/sql/schema_down.sqldrivers/pg/query/sql/schema_up.sqlintegration/testdata/cases/multipart_inline.json
✅ Files skipped from review due to trivial changes (4)
- cypher/models/pgsql/functions.go
- cypher/models/pgsql/model.go
- cypher/models/pgsql/format/format_test.go
- cypher/models/pgsql/translate/traversal.go
🚧 Files skipped from review as they are similar to previous changes (13)
- .gitignore
- cypher/models/pgsql/translate/selectivity.go
- cypher/models/pgsql/translate/limit_pushdown_test.go
- cypher/models/pgsql/translate/references_test.go
- cypher/models/pgsql/test/translation_cases/nodes.sql
- cypher/models/pgsql/translate/constraints_test.go
- cypher/models/pgsql/translate/references.go
- cypher/models/pgsql/test/translation_cases/pattern_expansion.sql
- cypher/models/pgsql/test/validation_integration_test.go
- drivers/pg/query/sql/schema_down.sql
- cypher/models/pgsql/translate/model.go
- cypher/models/pgsql/test/translation_cases/update.sql
- drivers/pg/query/sql/schema_up.sql
zinic
force-pushed
the
main
branch
2 times, most recently
from
e2eb55c to
98aac63
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cypher/models/pgsql/translate/model.go (1)
313-322:⚠️ Potential issue | 🟠 MajorPair-aware bidirectional gating still requires both endpoints to anchor.
This path only returns
truewhen the primer side clears the anchor threshold and the terminal side clears it too. The pair-aware gate is supposed to open once either endpoint is selective enough, so anchored+broader endpoint pairs still fall back to the unidirectional harness here.Suggested fix
- if primerSelectivity, err := s.endpointSelectivity(scope, s.Expansion.PrimerNodeConstraints, s.LeftNodeBound); err != nil { - return false, err - } else if !isBidirectionalSearchAnchor(primerSelectivity) { - return false, nil - } - - if terminalSelectivity, err := s.endpointSelectivity(scope, s.Expansion.TerminalNodeConstraints, s.RightNodeBound); err != nil { - return false, err - } else { - return isBidirectionalSearchAnchor(terminalSelectivity), nil - } + primerSelectivity, err := s.endpointSelectivity(scope, s.Expansion.PrimerNodeConstraints, s.LeftNodeBound) + if err != nil { + return false, err + } + + terminalSelectivity, err := s.endpointSelectivity(scope, s.Expansion.TerminalNodeConstraints, s.RightNodeBound) + if err != nil { + return false, err + } + + return isBidirectionalSearchAnchor(primerSelectivity) || + isBidirectionalSearchAnchor(terminalSelectivity), nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypher/models/pgsql/translate/model.go` around lines 313 - 322, The logic in the block using endpointSelectivity/PrimerNodeConstraints/LeftNodeBound and endpointSelectivity/TerminalNodeConstraints/RightNodeBound incorrectly requires both endpoints to be anchors; change it so that after computing primerSelectivity you immediately return true,nil if isBidirectionalSearchAnchor(primerSelectivity) is true, otherwise fall through to compute terminalSelectivity and return isBidirectionalSearchAnchor(terminalSelectivity),nil (preserving error propagation on each endpointSelectivity call). Ensure you reference and update the code paths around endpointSelectivity, s.Expansion.PrimerNodeConstraints, s.LeftNodeBound, s.Expansion.TerminalNodeConstraints, s.RightNodeBound, and isBidirectionalSearchAnchor.
🧹 Nitpick comments (6)
drivers/pg/query/sql/schema_down.sql (1)
19-19: 💤 Low valueAdd empty parentheses to bare-name
DROP FUNCTIONstatements for explicit clarity.
drop function if exists create_traversal_filter_tables;(line 19) targets the zero-argument overload specifically. While this works correctly and the three overloads with arguments are dropped explicitly on lines 20–22, adding empty parentheses makes the intent unambiguous and protects against future changes that might alter the overload set:Suggested change
-drop function if exists create_traversal_filter_tables; +drop function if exists create_traversal_filter_tables();The same applies to
create_bidirectional_shortest_path_tablesandcreate_bidirectional_pair_pathspace_indexeson lines 49–50.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/pg/query/sql/schema_down.sql` at line 19, Change the bare-name DROP FUNCTION statements to explicitly target the zero-argument overload by adding empty parentheses: replace occurrences like "drop function if exists create_traversal_filter_tables;" with "drop function if exists create_traversal_filter_tables();", and do the same for create_bidirectional_shortest_path_tables and create_bidirectional_pair_pathspace_indexes so the DROP unambiguously refers to the zero-argument overload.cypher/models/pgsql/test/testcase.go (1)
208-208: ⚡ Quick winUse the provided
ctxinstead of a freshcontext.Background()in the live test path.
AssertLiveacceptsctx context.Contextand forwards it todriver.Runon line 213, but the newtranslate.Translatecall here still usescontext.Background(). In a live test the caller's context is the only way to enforce timeouts/cancellation around translation, and it should be honored consistently with the runtime call.♻️ Proposed change
- if translation, err := translate.Translate(context.Background(), regularQuery, driver.KindMapper(), s.CypherParams, defaultGraph.ID); err != nil { + if translation, err := translate.Translate(ctx, regularQuery, driver.KindMapper(), s.CypherParams, defaultGraph.ID); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypher/models/pgsql/test/testcase.go` at line 208, The translate.Translate call is using context.Background() instead of the test's provided ctx; update the call to pass the incoming ctx (the same context that AssertLive receives and that is passed to driver.Run) so translation honors cancellation/timeouts. Locate the translate.Translate invocation (it currently passes context.Background(), regularQuery, driver.KindMapper(), s.CypherParams, defaultGraph.ID) and replace the context.Background() argument with ctx.cypher/models/pgsql/translate/limit_pushdown_test.go (1)
1-206: 💤 Low valueSolid coverage of the pushdown gating logic.
The helpers cleanly separate frame/CTE construction from the predicate shapes, and the test matrix exercises the main "allow" cases (forward & reversed endpoint inequality, bidirectional harness) plus two "block" cases (mixed where predicate, pre-filtered source CTE). The two
pushDownShortestPathLimit*tests pin the user-visible contract (a singleInt8-cast harness parameter equal topart.Limit) for both unidirectional and bidirectional harnesses.One small gap worth considering as a follow-up: there's no negative-path coverage for the bidirectional harness equivalent of
TestLimitPushdownTailSourceBlocksMixedShortestPathWherePredicate/TestLimitPushdownTailSourceBlocksFilteredShortestPathSource. If the bidirectional harness ever diverges in its gating heuristics, the unidirectional-only block tests won't catch it. Easy to add by parameterizing the existing block tests over bothpgsql.FunctionUnidirectionalSPHarnessandpgsql.FunctionBidirectionalSPHarness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypher/models/pgsql/translate/limit_pushdown_test.go` around lines 1 - 206, Duplicate/parameterize the two "block" tests (TestLimitPushdownTailSourceBlocksMixedShortestPathWherePredicate and TestLimitPushdownTailSourceBlocksFilteredShortestPathSource) to run for both pgsql.FunctionUnidirectionalSPHarness and pgsql.FunctionBidirectionalSPHarness: call limitPushdownTestPart with each harness, build the same tailSelects via limitPushdownTestTail and limitPushdownTestEndpointInequality (or manipulated source CTE), invoke limitPushdownTailSource, and require.False on canPushDown for both harnesses; this ensures the bidirectional harness is covered without changing existing helpers like limitPushdownTestPart, limitPushdownTestTail, or limitPushdownTailSource.cypher/models/pgsql/translate/unwind.go (1)
10-24: 💤 Low valueMinor:
deletehappens beforeDefineNewmay fail.
prepareUnwindTargetremovesvariablefroms.unwindTargetson line 15 unconditionally, but only later attemptss.scope.DefineNew(...)on line 17. IfDefineNewfails, the translator already lost the "this is an unwind target" bit, so any retry/recovery path (or higher-level diagnostics that walksunwindTargets) will see the variable as already consumed. Safer to delete only after a successfulDefineNew/Alias, e.g.:♻️ Proposed change
func (s *Translator) prepareUnwindTarget(variable *cypher.Variable) (*BoundIdentifier, bool, error) { if _, isUnwindTarget := s.unwindTargets[variable]; !isUnwindTarget { return nil, false, nil } - delete(s.unwindTargets, variable) - binding, err := s.scope.DefineNew(pgsql.UnsetDataType) if err != nil { return nil, true, err } s.scope.Alias(pgsql.Identifier(variable.Symbol), binding) + delete(s.unwindTargets, variable) return binding, true, nil }In practice this is unlikely to matter (
DefineNewrarely fails for a fresh untyped binding) — flagging for hardening only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypher/models/pgsql/translate/unwind.go` around lines 10 - 24, prepareUnwindTarget currently deletes the entry from s.unwindTargets before calling s.scope.DefineNew and s.scope.Alias; if DefineNew fails the unwind target is lost. Change the logic in prepareUnwindTarget so you only delete from s.unwindTargets after DefineNew returns successfully and after s.scope.Alias is called (i.e., move the delete(s.unwindTargets, variable) to after the binding is created and aliased) and ensure you still return the correct (binding, true, err) on error without mutating s.unwindTargets.cypher/models/pgsql/translate/expansion_test.go (1)
94-140: 💤 Low valueUseful pruning coverage; consider tightening the string-
Containsassertions.The two tests pin the right invariant — seed-local predicates (
lower(nX.id)::text = ...) stay inside the recursive primer while external predicates ((s0.x).id) get hoisted to the outer harness call. That's exactly the pruning gate this PR introduces.A couple of follow-up hardenings worth considering:
require.NotContains(t, primerQuery, "s0")on line 108/133 is overbroad — any future identifier that incidentally contains"s0"(e.g."s0_seed", a column literal, a generated suffix) would silently violate the intent without failing. The line 109/134(s0.x).idcheck is the real assertion; line 108/133 could be removed or replaced with a more specific frame-reference regex.require.Containson the formatted SQL is fragile to whitespace/parenthesization tweaks in the formatter. Ifformat.Statementever normalizes spacing differently, these tests will break for unrelated reasons. A normalization helper (collapse whitespace, lowercase) before substring matching would make them more robust, similar to whattestcase.goalready does.Neither blocks merge — just easy resilience wins.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypher/models/pgsql/translate/expansion_test.go` around lines 94 - 140, The NotContains assertions that check primerQuery/backwardPrimerQuery for the substring "s0" are overbroad and fragile; in the tests TestBoundRootShortestPathPrimerKeepsOnlySeedLocalConstraints and TestBoundTerminalShortestPathPrimerKeepsOnlySeedLocalConstraints replace/remove those broad checks on primerQuery/backwardPrimerQuery (derived from expansionModel.PrimerQueryParameter and expansionModel.BackwardPrimerQueryParameter) and instead assert a more specific frame reference (e.g. assert absence of "(s0." or use a word-boundary regex like `\bs0\b`) and keep the existing explicit `(s0.x).id` check; also make the formatted SQL assertions around format.Statement more robust by normalizing the formattedQuery (collapse whitespace and lowercase) before doing require.Contains on strings like "n0.id = (s0.x).id" and "(s0.n0).id = s1.root_id" (and the n1 variants) to avoid whitespace/parenthesization brittleness.cypher/models/pgsql/translate/create.go (1)
416-430: 💤 Low valueDefensive check:
kindIDs[0]can panic ifAssertKindsreturns an empty slice without error.The length guards above (
len(edgeCreate.Kinds) == 0/> 1) constrain input, butkindIDs[0]still assumes the mapper returned a non-empty slice. IfAssertKindsever returns(nil, nil)for an unmapped kind, this will index out of range.🛡️ Optional defensive guard
if kindIDs, err := s.kindMapper.AssertKinds(edgeCreate.Kinds); err != nil { return nil, fmt.Errorf("failed to translate edge kind: %w", err) + } else if len(kindIDs) == 0 { + return nil, fmt.Errorf("kind mapper returned no IDs for edge kind: %s", edgeCreate.Kinds[0]) } else { return pgsql.NewLiteral(kindIDs[0], pgsql.Int2), nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypher/models/pgsql/translate/create.go` around lines 416 - 430, In buildEdgeKindIDExpression, guard against AssertKinds returning an empty slice: after calling s.kindMapper.AssertKinds(edgeCreate.Kinds) check err and then verify len(kindIDs) > 0, returning a descriptive error (e.g., "no kind IDs returned for kinds: %v") if empty; only then use kindIDs[0] when constructing pgsql.NewLiteral. This references the Translator.buildEdgeKindIDExpression function, the AssertKinds call, the kindIDs slice, and pgsql.NewLiteral.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cypher/models/pgsql/test/translation_cases/update.sql`:
- Around line 50-54: The test file contains a duplicated test case for "match
(a)-[r]->(:NodeKind1) set a.name = '123', r.is_special_outbound = true" (the
identical WITH...SELECT SQL block appears twice); remove the duplicate block or
replace one with the intended unique case, ensuring only one occurrence of the
SQL block that selects/updates node n0 and edge e0 (the WITH s0 ... s1 ... s2
... select 1; sequence) remains, and run tests to confirm no other duplicates
exist.
In `@cypher/models/pgsql/translate/projection.go`:
- Around line 697-699: The current check in shortestPathEndpointAliases
(projection.go) treats any selectBody.Where as opaque and returns false,
preventing shortestPathLimitPushdownTransparentWhere from recognizing CTEs that
only contain the injected bound-endpoint and seed predicates; update
shortestPathEndpointAliases to allow a non-nil selectBody.Where when the WHERE
clause consists solely of the known injected predicate shapes (the
bound-endpoint and seed predicates inserted by the shortest-path builders) by
inspecting the WHERE expression(s) and returning true if every conjunct matches
those predicate patterns, otherwise keep returning false; reference
selectBody.Where, shortestPathEndpointAliases, and
shortestPathLimitPushdownTransparentWhere when locating and implementing this
conditional whitelist logic.
In `@drivers/pg/query/sql/schema_up.sql`:
- Around line 629-631: In swap_forward_front and swap_backward_front the
dead-end pruning uses the wrong edge direction: update the NOT EXISTS predicate
so swap_forward_front deletes rows only when no outgoing edges exist by changing
the inner condition to e.start_id = r.next_id, and update swap_backward_front to
check for incoming edges by changing its inner condition to e.end_id =
r.next_id; keep the rest of the delete predicates (is_cycle and satisfied
checks) unchanged.
---
Duplicate comments:
In `@cypher/models/pgsql/translate/model.go`:
- Around line 313-322: The logic in the block using
endpointSelectivity/PrimerNodeConstraints/LeftNodeBound and
endpointSelectivity/TerminalNodeConstraints/RightNodeBound incorrectly requires
both endpoints to be anchors; change it so that after computing
primerSelectivity you immediately return true,nil if
isBidirectionalSearchAnchor(primerSelectivity) is true, otherwise fall through
to compute terminalSelectivity and return
isBidirectionalSearchAnchor(terminalSelectivity),nil (preserving error
propagation on each endpointSelectivity call). Ensure you reference and update
the code paths around endpointSelectivity, s.Expansion.PrimerNodeConstraints,
s.LeftNodeBound, s.Expansion.TerminalNodeConstraints, s.RightNodeBound, and
isBidirectionalSearchAnchor.
---
Nitpick comments:
In `@cypher/models/pgsql/test/testcase.go`:
- Line 208: The translate.Translate call is using context.Background() instead
of the test's provided ctx; update the call to pass the incoming ctx (the same
context that AssertLive receives and that is passed to driver.Run) so
translation honors cancellation/timeouts. Locate the translate.Translate
invocation (it currently passes context.Background(), regularQuery,
driver.KindMapper(), s.CypherParams, defaultGraph.ID) and replace the
context.Background() argument with ctx.
In `@cypher/models/pgsql/translate/create.go`:
- Around line 416-430: In buildEdgeKindIDExpression, guard against AssertKinds
returning an empty slice: after calling
s.kindMapper.AssertKinds(edgeCreate.Kinds) check err and then verify
len(kindIDs) > 0, returning a descriptive error (e.g., "no kind IDs returned for
kinds: %v") if empty; only then use kindIDs[0] when constructing
pgsql.NewLiteral. This references the Translator.buildEdgeKindIDExpression
function, the AssertKinds call, the kindIDs slice, and pgsql.NewLiteral.
In `@cypher/models/pgsql/translate/expansion_test.go`:
- Around line 94-140: The NotContains assertions that check
primerQuery/backwardPrimerQuery for the substring "s0" are overbroad and
fragile; in the tests
TestBoundRootShortestPathPrimerKeepsOnlySeedLocalConstraints and
TestBoundTerminalShortestPathPrimerKeepsOnlySeedLocalConstraints replace/remove
those broad checks on primerQuery/backwardPrimerQuery (derived from
expansionModel.PrimerQueryParameter and
expansionModel.BackwardPrimerQueryParameter) and instead assert a more specific
frame reference (e.g. assert absence of "(s0." or use a word-boundary regex like
`\bs0\b`) and keep the existing explicit `(s0.x).id` check; also make the
formatted SQL assertions around format.Statement more robust by normalizing the
formattedQuery (collapse whitespace and lowercase) before doing require.Contains
on strings like "n0.id = (s0.x).id" and "(s0.n0).id = s1.root_id" (and the n1
variants) to avoid whitespace/parenthesization brittleness.
In `@cypher/models/pgsql/translate/limit_pushdown_test.go`:
- Around line 1-206: Duplicate/parameterize the two "block" tests
(TestLimitPushdownTailSourceBlocksMixedShortestPathWherePredicate and
TestLimitPushdownTailSourceBlocksFilteredShortestPathSource) to run for both
pgsql.FunctionUnidirectionalSPHarness and pgsql.FunctionBidirectionalSPHarness:
call limitPushdownTestPart with each harness, build the same tailSelects via
limitPushdownTestTail and limitPushdownTestEndpointInequality (or manipulated
source CTE), invoke limitPushdownTailSource, and require.False on canPushDown
for both harnesses; this ensures the bidirectional harness is covered without
changing existing helpers like limitPushdownTestPart, limitPushdownTestTail, or
limitPushdownTailSource.
In `@cypher/models/pgsql/translate/unwind.go`:
- Around line 10-24: prepareUnwindTarget currently deletes the entry from
s.unwindTargets before calling s.scope.DefineNew and s.scope.Alias; if DefineNew
fails the unwind target is lost. Change the logic in prepareUnwindTarget so you
only delete from s.unwindTargets after DefineNew returns successfully and after
s.scope.Alias is called (i.e., move the delete(s.unwindTargets, variable) to
after the binding is created and aliased) and ensure you still return the
correct (binding, true, err) on error without mutating s.unwindTargets.
In `@drivers/pg/query/sql/schema_down.sql`:
- Line 19: Change the bare-name DROP FUNCTION statements to explicitly target
the zero-argument overload by adding empty parentheses: replace occurrences like
"drop function if exists create_traversal_filter_tables;" with "drop function if
exists create_traversal_filter_tables();", and do the same for
create_bidirectional_shortest_path_tables and
create_bidirectional_pair_pathspace_indexes so the DROP unambiguously refers to
the zero-argument overload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a7d75d9-17bb-4a02-8d5e-ad8f84afb072
📒 Files selected for processing (44)
.gitignorecypher/models/pgsql/format/format.gocypher/models/pgsql/format/format_test.gocypher/models/pgsql/functions.gocypher/models/pgsql/model.gocypher/models/pgsql/test/testcase.gocypher/models/pgsql/test/translation_cases/create.sqlcypher/models/pgsql/test/translation_cases/delete.sqlcypher/models/pgsql/test/translation_cases/multipart.sqlcypher/models/pgsql/test/translation_cases/nodes.sqlcypher/models/pgsql/test/translation_cases/pattern_binding.sqlcypher/models/pgsql/test/translation_cases/pattern_expansion.sqlcypher/models/pgsql/test/translation_cases/quantifiers.sqlcypher/models/pgsql/test/translation_cases/shortest_paths.sqlcypher/models/pgsql/test/translation_cases/stepwise_traversal.sqlcypher/models/pgsql/test/translation_cases/unwind.sqlcypher/models/pgsql/test/translation_cases/update.sqlcypher/models/pgsql/test/validation_integration_test.gocypher/models/pgsql/translate/constraints_test.gocypher/models/pgsql/translate/create.gocypher/models/pgsql/translate/expansion.gocypher/models/pgsql/translate/expansion_test.gocypher/models/pgsql/translate/hinting.gocypher/models/pgsql/translate/limit_pushdown_test.gocypher/models/pgsql/translate/model.gocypher/models/pgsql/translate/node.gocypher/models/pgsql/translate/pattern.gocypher/models/pgsql/translate/projection.gocypher/models/pgsql/translate/query.gocypher/models/pgsql/translate/references.gocypher/models/pgsql/translate/references_test.gocypher/models/pgsql/translate/relationship.gocypher/models/pgsql/translate/selectivity.gocypher/models/pgsql/translate/tracking.gocypher/models/pgsql/translate/translator.gocypher/models/pgsql/translate/traversal.gocypher/models/pgsql/translate/unwind.gocypher/models/pgsql/translate/with.godrivers/pg/query/sql/schema_down.sqldrivers/pg/query/sql/schema_up.sqldrivers/pg/transaction.gointegration/testdata/cases/create_inline.jsonintegration/testdata/cases/multipart_inline.jsonintegration/testdata/cases/unwind_inline.json
✅ Files skipped from review due to trivial changes (1)
- integration/testdata/cases/unwind_inline.json
🚧 Files skipped from review as they are similar to previous changes (3)
- cypher/models/pgsql/translate/references.go
- cypher/models/pgsql/translate/constraints_test.go
- cypher/models/pgsql/test/translation_cases/quantifiers.sql
zinic
force-pushed
the
main
branch
5 times, most recently
from
2856185 to
c92ade8
Compare
Contributor
Author
|
@CodeRabbit blast me - review for correctness |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 74 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 2112 🏁 Script executed: Length of output: 2853 🏁 Script executed: Length of output: 990 🏁 Script executed: Length of output: 2157 🏁 Script executed: Length of output: 5336 🏁 Script executed: Length of output: 664 🏁 Script executed: Length of output: 4339 🏁 Script executed: Length of output: 10921 🏁 Script executed: Length of output: 223 🏁 Script executed: Length of output: 4014 🏁 Script executed: Length of output: 4136 🏁 Script executed: Length of output: 744 🏁 Script executed: Length of output: 1226 🏁 Script executed: Length of output: 3673 🏁 Script executed: Length of output: 865 🏁 Script executed: Length of output: 525 🏁 Script executed: Length of output: 1334 🏁 Script executed: Length of output: 14481 🏁 Script executed: Length of output: 4684 🏁 Script executed: Length of output: 352 🏁 Script executed: Length of output: 52 🏁 Script executed: Length of output: 352 🏁 Script executed: Length of output: 160 🏁 Script executed: Length of output: 387 🏁 Script executed: Length of output: 1777 🏁 Script executed: Length of output: 418 🏁 Script executed: Length of output: 648 🏁 Script executed: Length of output: 3393 🏁 Script executed: Length of output: 177 🏁 Script executed: Length of output: 584 🏁 Script executed: Length of output: 2518 🏁 Script executed: Length of output: 678 🏁 Script executed: Length of output: 3883 🏁 Script executed: Length of output: 1964 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 329 🏁 Script executed: Length of output: 1069 🏁 Script executed: Length of output: 459 🏁 Script executed: Length of output: 6692 🏁 Script executed: Length of output: 1909 🏁 Script executed: Length of output: 3514
Correctness Review🔴 Issue 1 —
|
zinic
force-pushed
the
main
branch
2 times, most recently
from
464418d to
a18cd0e
Compare
…ort CREATE, support UNWIND
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
1 participant
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.
Tighten CySQL traversal translation so variable-length and shortest-path expansions avoid broad frontier work unless the query gives the translator a safe anchor.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores