feat: introduce e2e cypher validation integration test suite#49
Open
zinic wants to merge 3 commits intoSpecterOps:mainfrom
Open
feat: introduce e2e cypher validation integration test suite#49zinic wants to merge 3 commits intoSpecterOps:mainfrom
zinic wants to merge 3 commits intoSpecterOps:mainfrom
Conversation
…te based on the existing static test suite cases
|
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 PostgreSQL-backed semantic integration tests and fixtures, a replayable result-assertion test utility, translator changes to emit and thread flat Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Semantic Test Runner
participant Schema as Schema Validator
participant Translator as Query Translator
participant DB as PostgreSQL (pg pool)
participant Executor as Query Executor
participant Asserter as Result Asserter
Test->>Schema: Assert SemanticGraphSchema
Schema-->>Test: Schema OK
Test->>Translator: Translate tc.Cypher (emit composite + <id>_id)
Translator->>Translator: build SQL with flat IDs
Translator-->>Executor: SQL
Executor->>DB: Execute SQL
DB-->>Executor: Rows (composite + scalar ids)
Executor-->>Test: graph.Result
Test->>Asserter: Apply tc.Assert
Asserter->>Asserter: Buffer/replay rows as needed
Asserter-->>Test: Assertion pass/fail
Test->>DB: Rollback fixture (errFixtureRollback)
DB-->>Test: Rolled back
sequenceDiagram
participant Translator as Translator
participant Projection as Projection Builder
participant Renamer as Identifier Rewriter
participant SQL as Generated SQL
Translator->>Projection: buildProjectionForNodeComposite(node, alias)
Projection->>Projection: Emit "node AS alias"
Projection->>Projection: Emit "node.id AS alias_id"
Projection->>Projection: Set binding.HasFlatID = true
Projection-->>Translator: Dual-column binding
Translator->>Renamer: rewrite reference to node id
alt HasFlatID == true
Renamer-->>SQL: use "<alias>_id" in predicates/joins
else
Renamer-->>SQL: use composite id extraction expression
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
779-799:⚠️ Potential issue | 🔴 CriticalDon't introduce the previous frame twice in the primer
FROM.When
primerExternal != nil, the block above already prependstraversalStep.Frame.Previousintoexpansion.PrimerStatement.From. ReplacingnextQueryFrom.Sourcewith that same frame here makes bound expansions render the prior CTE twice (FROM s0, s0 JOIN ...), which can duplicate rows or produce invalid SQL.Also applies to: 834-834
🤖 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 779 - 799, The primer frame (traversalStep.Frame.Previous) is being added twice when primerExternal != nil because nextQueryFrom.Source is unconditionally set to that same frame; change the construction of nextQueryFrom so you only set Source = traversalStep.Frame.Previous when primerExternal == nil (otherwise leave Source empty or use the existing primerExternal-driven From), and apply the same conditional fix at the other occurrence around the block referenced (the similar assignment at ~834). This prevents emitting the prior CTE twice in expansion.PrimerStatement.From and avoids duplicate FROM entries.
🧹 Nitpick comments (1)
cypher/models/pgsql/test/fixture.go (1)
33-43: Avoid maintaining two copies of the semantic schema.This schema is also hard-coded in
cypher/models/pgsql/test/validation_integration_test.go, and the comment here already calls out that kind ordering must stay identical. A one-sided edit will silently change kind IDs for one manual suite but not the other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/test/fixture.go` around lines 33 - 43, The SemanticGraphSchema variable is duplicated across fixture.go and validation_integration_test.go causing fragile kind ID drift; consolidate into a single source by moving the graph.Schema literal (SemanticGraphSchema) into a shared test helper package or a single test-only file and have validation_integration_test.go import/use that same SemanticGraphSchema symbol (or reference graph.Schema from the shared file) so NodeKind1, NodeKind2, EdgeKind1, EdgeKind2 and DefaultGraph remain identical across both test suites.
🤖 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/test/cases.go`:
- Around line 1726-1743: The PostAssert in the "detach-delete a typed node and
its incident edges" case only checks for query errors and doesn't assert the
node was removed; update the fixture to give the node a unique test-only
property (e.g., add a unique id when calling NewNodeWithProperties) and change
the PostAssert (the anonymous func using tx.Query) to query by that unique
property and assert that result.Next() returns false (and then check
result.Error()) so the test fails if the node still exists.
---
Outside diff comments:
In `@cypher/models/pgsql/translate/expansion.go`:
- Around line 779-799: The primer frame (traversalStep.Frame.Previous) is being
added twice when primerExternal != nil because nextQueryFrom.Source is
unconditionally set to that same frame; change the construction of nextQueryFrom
so you only set Source = traversalStep.Frame.Previous when primerExternal == nil
(otherwise leave Source empty or use the existing primerExternal-driven From),
and apply the same conditional fix at the other occurrence around the block
referenced (the similar assignment at ~834). This prevents emitting the prior
CTE twice in expansion.PrimerStatement.From and avoids duplicate FROM entries.
---
Nitpick comments:
In `@cypher/models/pgsql/test/fixture.go`:
- Around line 33-43: The SemanticGraphSchema variable is duplicated across
fixture.go and validation_integration_test.go causing fragile kind ID drift;
consolidate into a single source by moving the graph.Schema literal
(SemanticGraphSchema) into a shared test helper package or a single test-only
file and have validation_integration_test.go import/use that same
SemanticGraphSchema symbol (or reference graph.Schema from the shared file) so
NodeKind1, NodeKind2, EdgeKind1, EdgeKind2 and DefaultGraph remain identical
across both test suites.
🪄 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: 77df0d40-c070-4cae-aa02-551b0c516401
📒 Files selected for processing (7)
cypher/models/pgsql/test/assertions.gocypher/models/pgsql/test/cases.gocypher/models/pgsql/test/fixture.gocypher/models/pgsql/test/semantic_integration_test.gocypher/models/pgsql/test/translation_test.gocypher/models/pgsql/test/validation_integration_test.gocypher/models/pgsql/translate/expansion.go
💤 Files with no reviewable changes (1)
- cypher/models/pgsql/test/translation_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cypher/models/pgsql/test/assertions.go (2)
115-124: Consider explicit type assertion for consistency with other scalar assertions.Unlike
AssertAtLeastInt64andAssertExactInt64which explicitly type-assert and provide clear error messages when the type doesn't match, this function relies onrequire.Equalto compare directly. If the first column isn't a string, the error message may be less informative.♻️ Suggested improvement
func AssertScalarString(expected string) ResultAssertion { return func(t *testing.T, result graph.Result) { t.Helper() br := newBufferedResult(result) require.NoError(t, br.err) require.NotEmpty(t, br.rows, "no rows returned; cannot assert scalar string") - require.Equal(t, expected, br.rows[0][0], "scalar string mismatch") + val, ok := br.rows[0][0].(string) + require.True(t, ok, "expected string scalar, got %T: %v", br.rows[0][0], br.rows[0][0]) + require.Equal(t, expected, val, "scalar string mismatch") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/test/assertions.go` around lines 115 - 124, Update AssertScalarString to perform an explicit type assertion on the first column value (the value accessed in br.rows[0][0]) and fail with a clear error if it is not a string, rather than relying on require.Equal; locate the function AssertScalarString and replace the direct comparison with a type-assert (e.g., v, ok := br.rows[0][0].(string)) and use require.True/require.Fail or require.IsType with a descriptive message when the assertion fails, then compare v to expected.
141-161: Consider usingrequire.Failfor consistency with other assertions.Other assertion functions use
require.*which stops test execution on failure. Line 159 usest.Errorfwhich marks the test as failed but allows continued execution. While this works (it's the final statement), usingrequire.Failwould be more consistent with the rest of the file.♻️ Suggested improvement
} - t.Errorf("no result row contains a node with %s = %q", key, val) + require.Fail(t, "no result row contains a node with %s = %q", key, val) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/test/assertions.go` around lines 141 - 161, Replace the final failure call in AssertContainsNodeWithProp with the testify require failure helper to stop execution consistently: replace the t.Errorf(...) on the final line inside AssertContainsNodeWithProp with require.Fail (or require.Failf) using the same message/format so the test aborts like other assertions in this file; AssertContainsNodeWithProp is the function to update and no other logic should change.
🤖 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/test/translation_cases/pattern_binding.sql`:
- Line 45: The recursive seed in CTE s1 still performs an unconstrained join
"join node n2 on n2.id = e1.end_id" (node n2) which no longer matches the
expansion shape for the second leg (s2) or the translator's
TerminalNodeConstraints; either remove that join from the seed or add an
explicit terminal-node constraint that matches the rest of the expectation.
Locate the CTEs s1 and s2 in pattern_binding.sql (the inner seed that selects
from s0 join edge e1 ...) and edit the seed so it does not include "join node
n2" (or alternatively add a predicate constraining n2 to match the terminal-node
condition), then update downstream references (ep0/e1 assembly) accordingly so
the recursive seed matches the unconstrained "[:EdgeKind1*..]->()" expansion.
In `@cypher/models/pgsql/test/translation_cases/pattern_expansion.sql`:
- Line 48: The recursive seed in CTE s1/s2 still includes an unnecessary
right-node join (the "join node n2 on n2.id = e1.end_id" in the initial seed
select that uses e1/start_id from s0) which encodes a stale constrained seed;
remove that join so the seed is edge-only (keep "from s0 join edge e1 on
(s0.n1).id = e1.start_id" and drop the "join node n2" there), and do the same
for the analogous case at the other occurrence (drop the "join node n3" bound to
e*.end_id in the seed). Ensure any references to n2/n3 used only for filtering
in the seed are removed or relocated to the recursive member/outer query where
proper node joins remain.
---
Nitpick comments:
In `@cypher/models/pgsql/test/assertions.go`:
- Around line 115-124: Update AssertScalarString to perform an explicit type
assertion on the first column value (the value accessed in br.rows[0][0]) and
fail with a clear error if it is not a string, rather than relying on
require.Equal; locate the function AssertScalarString and replace the direct
comparison with a type-assert (e.g., v, ok := br.rows[0][0].(string)) and use
require.True/require.Fail or require.IsType with a descriptive message when the
assertion fails, then compare v to expected.
- Around line 141-161: Replace the final failure call in
AssertContainsNodeWithProp with the testify require failure helper to stop
execution consistently: replace the t.Errorf(...) on the final line inside
AssertContainsNodeWithProp with require.Fail (or require.Failf) using the same
message/format so the test aborts like other assertions in this file;
AssertContainsNodeWithProp is the function to update and no other logic should
change.
🪄 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: af5752be-a540-4008-923d-a6fad9b70ea6
📒 Files selected for processing (6)
cypher/models/pgsql/test/assertions.gocypher/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/translate/expansion.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cypher/models/pgsql/translate/expansion.go
cypher/models/pgsql/test/translation_cases/pattern_expansion.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
drivers/neo4j/batch_integration_test.go (2)
121-124:⚠️ Potential issue | 🔴 CriticalAssertion appears inverted:
HeatPropertywas deleted but test asserts it exists.At line 104, the property is deleted via
node.Properties.Delete(HeatProperty), but the assertion at line 122 checksassert.True(t, node.Properties.Exists(HeatProperty)). This will always fail.Should this assert that the property does not exist?
🐛 Proposed fix
for _, node := range nodes { - assert.True(t, node.Properties.Exists(HeatProperty)) + assert.False(t, node.Properties.Exists(HeatProperty)) assert.Equal(t, true, node.Properties.Get(NewProperty).Any()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drivers/neo4j/batch_integration_test.go` around lines 121 - 124, The test is asserting the wrong condition: after calling node.Properties.Delete(HeatProperty) the code still checks assert.True(t, node.Properties.Exists(HeatProperty)); change that assertion to assert.False(t, node.Properties.Exists(HeatProperty)) so the test verifies HeatProperty was removed, while keeping the existing check assert.Equal(t, true, node.Properties.Get(NewProperty).Any()) to ensure NewProperty remains present.
64-64:⚠️ Potential issue | 🟡 MinorTypo: "Regsiter" → "Register"
Minor typo in comment.
- // Regsiter a cleanup step to wipe the database + // Register a cleanup step to wipe the database🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drivers/neo4j/batch_integration_test.go` at line 64, Fix the typo in the comment that currently reads "Regsiter a cleanup step to wipe the database" — change "Regsiter" to "Register" so the comment reads "Register a cleanup step to wipe the database"; locate and update that comment (the one mentioning the cleanup step to wipe the database) in the batch integration test file.
🧹 Nitpick comments (1)
Makefile (1)
32-41: Consider adding.PHONYdeclarations and a trailing newline.The new targets work correctly, but two minor improvements would be beneficial:
- Add
.PHONYdeclarations for the new targets to ensure they always run (even if a file with that name exists).- Add a trailing newline at end of file (POSIX compliance).
♻️ Suggested improvement
Add near the top of the file with any existing
.PHONYdeclarations (or create one):.PHONY: test test_neo4j test_pg test_updateAnd ensure line 41 ends with a newline character.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 32 - 41, Add .PHONY declarations including the new targets so they always run even if files with those names exist: include .PHONY: test test_neo4j test_pg test_update (or append test_update to the existing .PHONY line) and ensure the Makefile ends with a trailing newline character; update the Makefile around the existing .PHONY block and the test_update target to include these changes.
🤖 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/test/translation_cases/multipart.sql`:
- Line 36: The query still uses composite dereferences like (n0).id, (n1).id,
(n3).id inside the same CTE/frame even though those frames already project flat
ids (n0_id, n1_id, n3_id); replace every same-frame composite dereference with
the corresponding flat id projection (e.g., change (n0).id -> n0_id, (n1).id ->
n1_id, (n3).id -> n3_id) in the s1/s3/s2 CTEs and any downstream references (so
the flat-id rewrite is exercised and HasFlatID exceptions aren’t bypassed), and
update the paired pgsql_params seed/step SQL that seeds/tests the query so its
id references match the replacements.
In `@cypher/models/pgsql/test/translation_cases/nodes.sql`:
- Line 307: The optional-match translation drops flat id columns (n0_id/n1_id)
from the outer-join frames s2/s4 and uses a plain equality when correlating
optional results (e.g., the predicate comparing s2.n1 = s3.n1), which causes
NULL = NULL to filter rows; fix by preserving the scalar id bindings (carry
n0_id and n1_id through s2 and s4 so the flat ids are available) and replace the
equality correlation with a null-stable comparison (use IS NOT DISTINCT FROM
between the appropriate id columns when correlating the OPTIONAL MATCH frames)
so the second optional match does not drop rows when the first optional match is
missing.
In `@cypher/models/pgsql/test/translation_cases/update.sql`:
- Line 57: The issue is the PostgreSQL string concatenation using '+' in the s1
CTE update statement (inside the UPDATE node n1 ... FROM s0) where the
expression 'n' + s0.n0_id is invalid; change it to use the text concatenation
operator and an explicit cast, e.g. replace the 'n' + s0.n0_id expression in the
UPDATE's jsonb_build_object call with 'n' || s0.n0_id::text so the name value is
properly concatenated as text.
In `@cypher/models/pgsql/translate/projection.go`:
- Around line 245-281: buildProjectionForExpansionNode sometimes emits two
SELECT items (composite + flat id) which breaks buildTailProjection's 1:1
pairing of currentPart.projections.Items with projection[i]; update
buildTailProjection so when it pairs projections it detects expanded projections
(use the projected.HasFlatID flag emitted by buildProjectionForExpansionNode or
the select-item count) and either consume/advance by two emitted items or
include the synthesized flat-id in the GROUP BY accordingly; adjust the loop
that walks currentPart.projections.Items vs projection[i] so it accounts for
multi-item projections (also apply same change for the similar logic covering
the 284-325 case).
---
Outside diff comments:
In `@drivers/neo4j/batch_integration_test.go`:
- Around line 121-124: The test is asserting the wrong condition: after calling
node.Properties.Delete(HeatProperty) the code still checks assert.True(t,
node.Properties.Exists(HeatProperty)); change that assertion to assert.False(t,
node.Properties.Exists(HeatProperty)) so the test verifies HeatProperty was
removed, while keeping the existing check assert.Equal(t, true,
node.Properties.Get(NewProperty).Any()) to ensure NewProperty remains present.
- Line 64: Fix the typo in the comment that currently reads "Regsiter a cleanup
step to wipe the database" — change "Regsiter" to "Register" so the comment
reads "Register a cleanup step to wipe the database"; locate and update that
comment (the one mentioning the cleanup step to wipe the database) in the batch
integration test file.
---
Nitpick comments:
In `@Makefile`:
- Around line 32-41: Add .PHONY declarations including the new targets so they
always run even if files with those names exist: include .PHONY: test test_neo4j
test_pg test_update (or append test_update to the existing .PHONY line) and
ensure the Makefile ends with a trailing newline character; update the Makefile
around the existing .PHONY block and the test_update target to include these
changes.
🪄 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: ffbcff8f-997f-4ae3-a722-7aa79212709a
📒 Files selected for processing (27)
MakefileREADME.mdcypher/models/pgsql/test/fixture.gocypher/models/pgsql/test/semantic_integration_test.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/parameters.sqlcypher/models/pgsql/test/translation_cases/pattern_binding.sqlcypher/models/pgsql/test/translation_cases/pattern_expansion.sqlcypher/models/pgsql/test/translation_cases/pattern_rewriting.sqlcypher/models/pgsql/test/translation_cases/quantifiers.sqlcypher/models/pgsql/test/translation_cases/scalar_aggregation.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/translation_integration_test.gocypher/models/pgsql/translate/match.gocypher/models/pgsql/translate/projection.gocypher/models/pgsql/translate/renamer.gocypher/models/pgsql/translate/tracking.gocypher/models/pgsql/translate/with.gocypher/test/cases/filtering_tests.jsoncypher/test/cases/mutation_tests.jsoncypher/test/cases/negative_tests.jsoncypher/test/cases/positive_tests.jsondrivers/neo4j/batch_integration_test.go
✅ Files skipped from review due to trivial changes (5)
- cypher/test/cases/filtering_tests.json
- cypher/test/cases/negative_tests.json
- cypher/test/cases/positive_tests.json
- README.md
- cypher/test/cases/mutation_tests.json
🚧 Files skipped from review as they are similar to previous changes (3)
- cypher/models/pgsql/test/translation_cases/pattern_binding.sql
- cypher/models/pgsql/test/semantic_integration_test.go
- cypher/models/pgsql/test/translation_cases/pattern_expansion.sql
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Makefile (1)
10-12: Consider adding.PHONYdeclarations for non-file targets.Static analysis suggests declaring
default,all, and other non-file targets as.PHONYto prevent conflicts with files of the same name and improve performance.♻️ Proposed enhancement
+.PHONY: default all build deps tidy lint format test test_bench test_neo4j test_pg test_update generate clean help + # Default target default: help all: generate format tidy lint test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 10 - 12, Add a .PHONY declaration listing non-file targets to avoid conflicts: declare .PHONY for targets like default, all, help, generate, format, tidy, lint, and test (and any other meta targets present) by adding a single .PHONY line that includes those target names so Make treats them as always out-of-date rather than as filenames.cypher/models/pgsql/translate/expansion.go (1)
822-867: Inconsistent condition for terminal node join:TerminalNodeSatisfactionProjectionvsTerminalNodeConstraints.The primer statement (Line 823) uses
TerminalNodeSatisfactionProjection != nilwhile the recursive statement (Line 856) usesTerminalNodeConstraints != nil. Although these are currently equivalent (sinceTerminalNodeSatisfactionProjectionis only set whenTerminalNodeConstraintsexists), using different conditions is confusing and fragile if that relationship changes.Consider using the same condition in both places for consistency and maintainability.
🤖 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 822 - 867, The two checks for adding the terminal-node join are inconsistent: the primer block uses expansionModel.TerminalNodeSatisfactionProjection != nil while the recursive block uses expansionModel.TerminalNodeConstraints != nil; make them use the same condition (prefer expansionModel.TerminalNodeConstraints != nil for clarity) so both the primer append to expansion.PrimerStatement.From (nextQueryFrom with traversalStep.RightNode.Identifier) and the recursiveJoins append use expansionModel.TerminalNodeConstraints != nil consistently.cypher/models/pgsql/test/semantic_integration_test.go (1)
53-56: UsetestCtxinstead ofcontext.TODO().
testCtxis already created and available on line 45, butcontext.TODO()is used here. Using the same context ensures consistent cancellation behavior.♻️ Suggested fix
- db, err := dawgs.Open(context.TODO(), pg.DriverName, dawgs.Config{ + db, err := dawgs.Open(testCtx, pg.DriverName, dawgs.Config{ GraphQueryMemoryLimit: size.Gibibyte, Pool: pgxPool, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/test/semantic_integration_test.go` around lines 53 - 56, Replace the use of context.TODO() in the dawgs.Open call with the existing testCtx to ensure consistent cancellation propagation; specifically, update the invocation of dawgs.Open(..., pg.DriverName, dawgs.Config{...}) so the first argument is testCtx instead of context.TODO(), leaving the pg.DriverName and dawgs.Config (GraphQueryMemoryLimit, Pool) as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cardinality/hyperloglog_bench_test.go`:
- Around line 365-368: The deviation calculation uses a signed percent error
which lets large underestimates pass the threshold; change the computation to an
absolute relative error. Replace the current divergence formula that builds
deviation from estimatedCardinality and cardinalityMax with an absolute percent
error, e.g. compute deviation =
math.Abs(float64(estimatedCardinality)-cardinalityMax)/cardinalityMax*100 (or
the equivalent) and use that value for the `< 0.5` / `< 0.66` assertions; update
both occurrences where deviation is computed (the block that defines
estimatedCardinality := sketch.Cardinality() and the other occurrence around the
later assertion).
In `@Makefile`:
- Around line 54-62: The Makefile target test_update contains a
duplicate/mistargeted cleanup: after copying from
cypher/models/pgsql/test/updated_cases the script incorrectly runs rm -rf
cypher/analyzer/updated_cases/ again; update the test_update target to remove
the duplicate line and instead clean the correct directory (remove the second rm
-rf cypher/analyzer/updated_cases/ and, if needed, replace it with rm -rf
cypher/models/pgsql/test/updated_cases/) so the cleanup matches the preceding cp
operations in the test_update target.
- Around line 101-102: The two stray commands "cp -fv
cypher/models/pgsql/test/updated_cases/*
cypher/models/pgsql/test/translation_cases" and "rm -rf
cypher/models/pgsql/test/updated_cases" are sitting outside any Makefile target
and should be removed; locate those exact commands (duplicates of the commands
in the test_update target) and delete them so no standalone shell commands exist
outside targets in the Makefile.
---
Nitpick comments:
In `@cypher/models/pgsql/test/semantic_integration_test.go`:
- Around line 53-56: Replace the use of context.TODO() in the dawgs.Open call
with the existing testCtx to ensure consistent cancellation propagation;
specifically, update the invocation of dawgs.Open(..., pg.DriverName,
dawgs.Config{...}) so the first argument is testCtx instead of context.TODO(),
leaving the pg.DriverName and dawgs.Config (GraphQueryMemoryLimit, Pool) as-is.
In `@cypher/models/pgsql/translate/expansion.go`:
- Around line 822-867: The two checks for adding the terminal-node join are
inconsistent: the primer block uses
expansionModel.TerminalNodeSatisfactionProjection != nil while the recursive
block uses expansionModel.TerminalNodeConstraints != nil; make them use the same
condition (prefer expansionModel.TerminalNodeConstraints != nil for clarity) so
both the primer append to expansion.PrimerStatement.From (nextQueryFrom with
traversalStep.RightNode.Identifier) and the recursiveJoins append use
expansionModel.TerminalNodeConstraints != nil consistently.
In `@Makefile`:
- Around line 10-12: Add a .PHONY declaration listing non-file targets to avoid
conflicts: declare .PHONY for targets like default, all, help, generate, format,
tidy, lint, and test (and any other meta targets present) by adding a single
.PHONY line that includes those target names so Make treats them as always
out-of-date rather than as filenames.
🪄 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: 500c675e-c36a-4e61-9d27-8a29a7de7258
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (32)
MakefileREADME.mdcardinality/hyperloglog32_test.gocardinality/hyperloglog64_test.gocardinality/hyperloglog_bench_test.gocypher/models/pgsql/test/fixture.gocypher/models/pgsql/test/semantic_integration_test.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/parameters.sqlcypher/models/pgsql/test/translation_cases/pattern_binding.sqlcypher/models/pgsql/test/translation_cases/pattern_expansion.sqlcypher/models/pgsql/test/translation_cases/pattern_rewriting.sqlcypher/models/pgsql/test/translation_cases/quantifiers.sqlcypher/models/pgsql/test/translation_cases/scalar_aggregation.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/translation_integration_test.gocypher/models/pgsql/translate/expansion.gocypher/models/pgsql/translate/match.gocypher/models/pgsql/translate/projection.gocypher/models/pgsql/translate/renamer.gocypher/models/pgsql/translate/tracking.gocypher/models/pgsql/translate/with.gocypher/test/cases/filtering_tests.jsoncypher/test/cases/mutation_tests.jsoncypher/test/cases/negative_tests.jsoncypher/test/cases/positive_tests.jsondrivers/neo4j/batch_integration_test.gogo.mod
💤 Files with no reviewable changes (3)
- cardinality/hyperloglog64_test.go
- cardinality/hyperloglog32_test.go
- go.mod
✅ Files skipped from review due to trivial changes (9)
- cypher/test/cases/mutation_tests.json
- cypher/test/cases/negative_tests.json
- cypher/test/cases/positive_tests.json
- README.md
- cypher/test/cases/filtering_tests.json
- cypher/models/pgsql/test/translation_cases/shortest_paths.sql
- cypher/models/pgsql/translate/projection.go
- cypher/models/pgsql/test/translation_cases/multipart.sql
- cypher/models/pgsql/test/fixture.go
🚧 Files skipped from review as they are similar to previous changes (10)
- cypher/models/pgsql/translate/with.go
- cypher/models/pgsql/translate/match.go
- drivers/neo4j/batch_integration_test.go
- cypher/models/pgsql/translate/tracking.go
- cypher/models/pgsql/test/translation_cases/quantifiers.sql
- cypher/models/pgsql/test/translation_cases/pattern_rewriting.sql
- cypher/models/pgsql/test/translation_cases/parameters.sql
- cypher/models/pgsql/test/translation_cases/nodes.sql
- cypher/models/pgsql/test/translation_cases/stepwise_traversal.sql
- cypher/models/pgsql/test/translation_cases/update.sql
Comment on lines
+365
to
+368
| var ( | ||
| estimatedCardinality = sketch.Cardinality() | ||
| deviation = 100 - cardinalityMax/float64(estimatedCardinality)*100 | ||
| ) |
There was a problem hiding this comment.
Deviation calculation can false-pass major underestimation.
Line 367 and Line 398 compute a signed error, so significant underestimates become negative and still satisfy the < 0.5 / < 0.66 assertions.
Proposed fix
import (
"encoding/binary"
+ "math"
"sync"
"testing"
@@
var (
estimatedCardinality = sketch.Cardinality()
- deviation = 100 - cardinalityMax/float64(estimatedCardinality)*100
+ deviation = math.Abs(float64(estimatedCardinality)-float64(cardinalityMax)) / float64(cardinalityMax) * 100
)
@@
var (
estimatedCardinality = sketch.Cardinality()
- deviation = 100 - cardinalityMax/float64(estimatedCardinality)*100
+ deviation = math.Abs(float64(estimatedCardinality)-float64(cardinalityMax)) / float64(cardinalityMax) * 100
)Also applies to: 396-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cardinality/hyperloglog_bench_test.go` around lines 365 - 368, The deviation
calculation uses a signed percent error which lets large underestimates pass the
threshold; change the computation to an absolute relative error. Replace the
current divergence formula that builds deviation from estimatedCardinality and
cardinalityMax with an absolute percent error, e.g. compute deviation =
math.Abs(float64(estimatedCardinality)-cardinalityMax)/cardinalityMax*100 (or
the equivalent) and use that value for the `< 0.5` / `< 0.66` assertions; update
both occurrences where deviation is computed (the block that defines
estimatedCardinality := sketch.Cardinality() and the other occurrence around the
later assertion).
zinic
force-pushed
the
main
branch
2 times, most recently
from
9362d80 to
d95d03a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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/operators.go (1)
33-43:⚠️ Potential issue | 🟠 MajorRegister
OperatorIsNotDistinctFromin operator classification helpers.Line 89 adds the constant, but
OperatorIsBooleanandOperatorIsComparatorstill omit it. This leavesIS NOT DISTINCT FROMoutside comparator/boolean paths and can break expression type inference.Proposed fix
func OperatorIsBoolean(operator Expression) bool { return OperatorIsIn(operator, OperatorAnd, OperatorOr, OperatorNot, OperatorEquals, OperatorNotEquals, OperatorGreaterThan, OperatorGreaterThanOrEqualTo, OperatorLessThan, - OperatorLessThanOrEqualTo) + OperatorLessThanOrEqualTo, + OperatorIsNotDistinctFrom) } func OperatorIsComparator(operator Expression) bool { return OperatorIsIn(operator, OperatorEquals, OperatorNotEquals, OperatorGreaterThan, OperatorGreaterThanOrEqualTo, OperatorLessThan, OperatorLessThanOrEqualTo, OperatorArrayOverlap, OperatorLike, OperatorILike, OperatorPGArrayOverlap, - OperatorRegexMatch, OperatorSimilarTo, OperatorPGArrayLHSContainsRHS) + OperatorRegexMatch, OperatorSimilarTo, OperatorPGArrayLHSContainsRHS, OperatorIsNotDistinctFrom) }Also applies to: 54-59, 89-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/operators.go` around lines 33 - 43, The OperatorIsNotDistinctFrom constant added on line 89 was not included in the operator classification helpers; update the OperatorIsBoolean and OperatorIsComparator functions to include OperatorIsNotDistinctFrom in their OperatorIsIn calls so the "IS NOT DISTINCT FROM" operator is treated as a comparator/boolean operator for expression type inference (locate and modify OperatorIsBoolean and OperatorIsComparator to add OperatorIsNotDistinctFrom to their argument lists).
🧹 Nitpick comments (3)
cypher/models/pgsql/test/semantic_integration_test.go (1)
52-62: Consider parallelizing subtests for faster execution.The test cases appear to be independent (each sets up its own fixture and rolls back). You could use
t.Parallel()within the subtest to run cases concurrently, which would significantly speed up the suite.⚡ Suggested change for parallel execution
for _, testCase := range semanticTestCases { + testCase := testCase // capture range variable t.Run(testCase.Name, func(t *testing.T) { + t.Parallel() if err := db.AssertSchema(testCtx, SemanticGraphSchema); err != nil { t.Fatalf("Failed asserting graph schema: %v", err) } - - numCasesRun += 1 - slog.Info("Semantic Test", slog.String("name", testCase.Name), slog.Int("num_cases_run", numCasesRun)) runSemanticCase(testCtx, t, db, testCase) }) }Note: If parallelization is added, the
numCasesRuncounter would need atomic operations or removal, andAssertSchemawould need to be verified as concurrency-safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/test/semantic_integration_test.go` around lines 52 - 62, Add parallel execution to the subtests by calling t.Parallel() at the start of each subtest closure (the anonymous func passed to t.Run for each semanticTestCases entry) so runSemanticCase and independent fixtures execute concurrently; update the numCasesRun usage to be either removed or guarded with atomic operations (e.g., atomic.AddInt32/Int64) to avoid races, and ensure the AssertSchema method on db (AssertSchema(testCtx, SemanticGraphSchema)) is concurrency-safe or otherwise run schema assertion once before starting parallel subtests.Makefile (1)
11-12: Consider adding.PHONYdeclarations for non-file targets.Static analysis flagged that
default,all, and other targets should be declared as phony since they don't produce files. This prevents issues if files with these names are accidentally created.📝 Suggested addition at the top of the file
.PHONY: default all build deps tidy lint format test test_bench test_neo4j test_pg test_update generate clean help🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 11 - 12, Add a .PHONY declaration listing all non-file Makefile targets so make treats them as phony (preventing conflicts if files with those names exist); include targets mentioned in the diff and review such as default, all, build, deps, tidy, lint, format, test, test_bench, test_neo4j, test_pg, test_update, generate, clean, and help in a single .PHONY line near the top of the Makefile (so targets like default and all are recognized as phony).cypher/models/pgsql/test/fixture.go (1)
21-31: Schema synchronization note is helpful but could be stronger.The comment warns about keeping
SemanticGraphSchemain sync withvalidation_integration_test.go, but there's no automated enforcement. If these drift, kind IDs will mismatch and tests will fail silently or produce confusing results.Consider adding a unit test that validates the schemas match, or centralizing the schema definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/test/fixture.go` around lines 21 - 31, The comment warns that SemanticGraphSchema can drift from the schema in validation_integration_test.go causing kind ID mismatches; add an automated check: implement a unit test (e.g., TestSemanticGraphSchemaMatchesValidationSchema) that loads/constructs the schema used in validation_integration_test.go and compares it to the SemanticGraphSchema variable (comparing Graphs, Nodes, Edges and DefaultGraph names/order), failing the test with a clear message if they differ; alternatively, refactor to centralize/return the canonical schema from a single function used by both places (expose GetSemanticGraphSchema used by SemanticGraphSchema and the validation test).
🤖 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/test/translation_cases/scalar_aggregation.sql`:
- Around line 63-66: The fixtures drop n0_id when projecting through the CTEs
(s1 -> s0) so the outer SELECTs that return s0.n0 as n and s0.i0 as
props/node_count lose the flat id; propagate n0_id from s1 into s0 and include
s0.n0_id in the final projections (e.g., select s0.n0 as n, s0.n0_id as n0_id,
s0.i0 as ...) and ensure any GROUP BY in the s0-producing SELECT still accounts
for n0_id (or include it in the s1->s0 projection) so the returned row shape
matches the other node+flat-id fixtures.
In `@Makefile`:
- Line 76: Update the Makefile help text for the all target so it accurately
reflects what the target does: change the line showing the description for "all"
(the help echo for the all target) to include tidy and lint (e.g., "Generate,
format, tidy, lint, and test") so the printed help matches the actual all target
dependencies.
---
Outside diff comments:
In `@cypher/models/pgsql/operators.go`:
- Around line 33-43: The OperatorIsNotDistinctFrom constant added on line 89 was
not included in the operator classification helpers; update the
OperatorIsBoolean and OperatorIsComparator functions to include
OperatorIsNotDistinctFrom in their OperatorIsIn calls so the "IS NOT DISTINCT
FROM" operator is treated as a comparator/boolean operator for expression type
inference (locate and modify OperatorIsBoolean and OperatorIsComparator to add
OperatorIsNotDistinctFrom to their argument lists).
---
Nitpick comments:
In `@cypher/models/pgsql/test/fixture.go`:
- Around line 21-31: The comment warns that SemanticGraphSchema can drift from
the schema in validation_integration_test.go causing kind ID mismatches; add an
automated check: implement a unit test (e.g.,
TestSemanticGraphSchemaMatchesValidationSchema) that loads/constructs the schema
used in validation_integration_test.go and compares it to the
SemanticGraphSchema variable (comparing Graphs, Nodes, Edges and DefaultGraph
names/order), failing the test with a clear message if they differ;
alternatively, refactor to centralize/return the canonical schema from a single
function used by both places (expose GetSemanticGraphSchema used by
SemanticGraphSchema and the validation test).
In `@cypher/models/pgsql/test/semantic_integration_test.go`:
- Around line 52-62: Add parallel execution to the subtests by calling
t.Parallel() at the start of each subtest closure (the anonymous func passed to
t.Run for each semanticTestCases entry) so runSemanticCase and independent
fixtures execute concurrently; update the numCasesRun usage to be either removed
or guarded with atomic operations (e.g., atomic.AddInt32/Int64) to avoid races,
and ensure the AssertSchema method on db (AssertSchema(testCtx,
SemanticGraphSchema)) is concurrency-safe or otherwise run schema assertion once
before starting parallel subtests.
In `@Makefile`:
- Around line 11-12: Add a .PHONY declaration listing all non-file Makefile
targets so make treats them as phony (preventing conflicts if files with those
names exist); include targets mentioned in the diff and review such as default,
all, build, deps, tidy, lint, format, test, test_bench, test_neo4j, test_pg,
test_update, generate, clean, and help in a single .PHONY line near the top of
the Makefile (so targets like default and all are recognized as phony).
🪄 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: abc72927-8738-4323-baa6-d686d00a5709
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
MakefileREADME.mdcardinality/hyperloglog32_test.gocardinality/hyperloglog64_test.gocardinality/hyperloglog_bench_test.gocypher/models/pgsql/operators.gocypher/models/pgsql/test/assertions.gocypher/models/pgsql/test/cases.gocypher/models/pgsql/test/fixture.gocypher/models/pgsql/test/semantic_integration_test.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/parameters.sqlcypher/models/pgsql/test/translation_cases/pattern_binding.sqlcypher/models/pgsql/test/translation_cases/pattern_expansion.sqlcypher/models/pgsql/test/translation_cases/pattern_rewriting.sqlcypher/models/pgsql/test/translation_cases/quantifiers.sqlcypher/models/pgsql/test/translation_cases/scalar_aggregation.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/translation_integration_test.gocypher/models/pgsql/translate/expansion.gocypher/models/pgsql/translate/match.gocypher/models/pgsql/translate/projection.gocypher/models/pgsql/translate/renamer.gocypher/models/pgsql/translate/tracking.gocypher/models/pgsql/translate/with.gocypher/test/cases/filtering_tests.jsoncypher/test/cases/mutation_tests.jsoncypher/test/cases/negative_tests.jsoncypher/test/cases/positive_tests.jsondrivers/neo4j/batch_integration_test.gogo.mod
💤 Files with no reviewable changes (3)
- go.mod
- cardinality/hyperloglog32_test.go
- cardinality/hyperloglog64_test.go
✅ Files skipped from review due to trivial changes (7)
- cypher/models/pgsql/translate/with.go
- cypher/test/cases/negative_tests.json
- cypher/test/cases/mutation_tests.json
- cypher/test/cases/positive_tests.json
- README.md
- cypher/test/cases/filtering_tests.json
- cypher/models/pgsql/translate/expansion.go
🚧 Files skipped from review as they are similar to previous changes (16)
- cypher/models/pgsql/test/translation_cases/parameters.sql
- cypher/models/pgsql/translate/projection.go
- cypher/models/pgsql/translate/renamer.go
- drivers/neo4j/batch_integration_test.go
- cypher/models/pgsql/translate/match.go
- cypher/models/pgsql/translate/tracking.go
- cypher/models/pgsql/test/translation_cases/pattern_rewriting.sql
- cypher/models/pgsql/test/translation_cases/shortest_paths.sql
- cypher/models/pgsql/test/translation_cases/stepwise_traversal.sql
- cypher/models/pgsql/test/translation_integration_test.go
- cypher/models/pgsql/test/assertions.go
- cypher/models/pgsql/test/translation_cases/multipart.sql
- cypher/models/pgsql/test/translation_cases/update.sql
- cypher/models/pgsql/test/translation_cases/pattern_binding.sql
- cypher/models/pgsql/test/cases.go
- cypher/models/pgsql/test/translation_cases/pattern_expansion.sql
cypher/models/pgsql/test/translation_cases/scalar_aggregation.sql
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@coderabbitai review blast me |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
drivers/neo4j/batch_integration_test.go (1)
64-64:⚠️ Potential issue | 🟡 MinorMinor typo: "Regsiter" → "Register".
📝 Proposed fix
- // Regsiter a cleanup step to wipe the database + // Register a cleanup step to wipe the database🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drivers/neo4j/batch_integration_test.go` at line 64, Fix the typo in the comment string "Regsiter a cleanup step to wipe the database" in drivers/neo4j/batch_integration_test.go by changing "Regsiter" to "Register" so the comment reads "Register a cleanup step to wipe the database"; locate the comment (exact text shown) and update it accordingly.cypher/models/pgsql/translate/projection.go (1)
526-550:⚠️ Potential issue | 🔴 CriticalIndex mapping will break when projections with flat IDs are present.
The loop at lines 528-546 iterates over
currentPart.projections.Itemswith indexiand accessesprojection[i], assuming 1:1 correspondence. However,buildProjection()can return multiple items per input projection:
buildProjectionForExpansionNode()andbuildProjectionForNodeComposite()return 2 items whenLastProjection != nil && HasFlatID(the composite plus a_idvariant appended at lines 258-261 and 297-300)- All other projection types return 1 item
Since
buildExternalProjection()accumulates items sequentially (line 50), the resultingprojectionslice grows unpredictably. Ifprojections.Items[0]produces 2 items andprojections.Items[1]produces 1 item, thenprojection[1]does not correspond toItems[1]but to the second item ofItems[0].This assumption must be preserved or the index-based lookup requires a mapping that tracks which slice indices correspond to which
Itemselements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/translate/projection.go` around lines 526 - 550, The code assumes a 1:1 mapping between currentPart.projections.Items and the flattened projection slice (projection), which breaks when buildExternalProjection expands a single input item into multiple output items (e.g., buildProjectionForExpansionNode / buildProjectionForNodeComposite producing a flat _id). Fix by changing buildExternalProjection to also return a mapping from each input projection item index to the list/range of indices in the flattened projection slice (e.g., projectionIndexMap map[int][]int or []int offsets per item), then update this loop to iterate currentPart.projections.Items and for each item use the mapping to collect all corresponding projExpr entries (checking each mapped index and appending aliasedExpr.Expression or projExpr to nonAggregateExprs as before) before assigning singlePartQuerySelect.GroupBy; reference buildExternalProjection, buildProjectionForExpansionNode, buildProjectionForNodeComposite, currentPart.projections.Items, projection, and singlePartQuerySelect.GroupBy.
♻️ Duplicate comments (4)
cardinality/hyperloglog_bench_test.go (1)
365-368:⚠️ Potential issue | 🟠 MajorUse absolute relative error for deviation checks.
Line 367 and Line 398 use a signed deviation formula, so large underestimates can become negative and incorrectly satisfy
< 0.5/< 0.66.Proposed fix
import ( "encoding/binary" + "math" "sync" "testing" "github.com/axiomhq/hyperloglog" "github.com/stretchr/testify/require" ) @@ var ( estimatedCardinality = sketch.Cardinality() - deviation = 100 - cardinalityMax/float64(estimatedCardinality)*100 + deviation = math.Abs(float64(estimatedCardinality)-float64(cardinalityMax)) / float64(cardinalityMax) * 100 ) @@ var ( estimatedCardinality = sketch.Cardinality() - deviation = 100 - cardinalityMax/float64(estimatedCardinality)*100 + deviation = math.Abs(float64(estimatedCardinality)-float64(cardinalityMax)) / float64(cardinalityMax) * 100 )Also applies to: 396-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cardinality/hyperloglog_bench_test.go` around lines 365 - 368, The deviation computation uses a signed formula so large underestimates produce negative values that can incorrectly pass the threshold; update the calculation to use absolute relative error (e.g. compute deviation as the absolute percentage difference between estimatedCardinality (from sketch.Cardinality()) and cardinalityMax: deviation = math.Abs(float64(estimatedCardinality)-cardinalityMax)/cardinalityMax*100) and replace the current deviation formula wherever used (references: estimatedCardinality, deviation, sketch.Cardinality(), cardinalityMax) so the comparisons against 0.5 / 0.66 thresholds correctly use the absolute error.cypher/models/pgsql/test/cases.go (1)
1710-1727:⚠️ Potential issue | 🟠 MajorMake the detach-delete post-check prove the fixture row disappeared.
The
PostAssertat Line 1720 only reruns a broadname = 'victim'query and never fails when a row is returned, so this case can still pass with a broken delete or fail because of unrelated data. Give the fixture a unique sentinel property and assert thatresult.Next()is false.♻️ Suggested fix
Fixture: GraphFixture{ Nodes: []NodeFixture{ - NewNodeWithProperties("victim", map[string]any{"name": "victim"}, NodeKind1), + NewNodeWithProperties("victim", map[string]any{ + "name": "victim", + "semantic_case": "detach-delete-victim", + }, NodeKind1), }, }, Assert: AssertNoError(), PostAssert: func(t *testing.T, tx graph.Transaction) { t.Helper() - result := tx.Query(`match (n:NodeKind1) where n.name = 'victim' return n`, nil) + result := tx.Query(`match (n:NodeKind1) where n.semantic_case = 'detach-delete-victim' return n`, nil) defer result.Close() - for result.Next() { - } + if result.Next() { + t.Fatalf("victim node still exists after detach delete") + } if err := result.Error(); err != nil { t.Errorf("post-delete query error: %v", err) } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/test/cases.go` around lines 1710 - 1727, The PostAssert for the "detach-delete a typed node and its incident edges" test (PostAssert function using tx.Query and iterating result.Next()) doesn't verify the fixture row was removed; update the fixture creation (NewNodeWithProperties for "victim", NodeKind1) to include a unique sentinel property (e.g. "sentinel": "unique-<something>") and change the PostAssert to run the same query filtering on that sentinel, then assert that result.Next() is false (and fail the test via t.Fatalf or t.Errorf if it returns true), while still checking result.Error() after closing the result.cypher/models/pgsql/translate/expansion.go (2)
14-25:⚠️ Potential issue | 🔴 CriticalThe shortest-path harness SQL still captures out-of-scope prior-frame references.
nodeIDRefonly fixes the outer projection predicate. The formatted@pi0/@pi1fragments are still built byprepareForwardFrontPrimerQuery/prepareForwardFrontRecursiveQuery, which never add the previous frame to the fragment’sFROM. Line 84 incypher/models/pgsql/test/translation_cases/multipart.sqlstill emitsjoin node n1 on (s0.n1).id = e1.start_idinside the harness SQL string, so bound-start shortest-path queries will still fail when the function executes that fragment. The harness fragments need the same previous-frame plumbing and flat-id rewrite as the outer query.Also applies to: 513-517, 600-604
🤖 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 14 - 25, The shortest-path harness fragments generated by prepareForwardFrontPrimerQuery and prepareForwardFrontRecursiveQuery still reference prior-frame node IDs without adding the prior frame to the fragment’s FROM or applying the flat-ID rewrite; update both functions so they build their `@pi0/`@pi1 fragments using the same plumbing as nodeIDRef: when the BoundIdentifier.HasFlatID is true emit a pgsql.CompoundIdentifier of the prior frame (frameID, Identifier + "_id"), otherwise call rewriteCompositeTypeFieldReference(frameID, pgsql.CompoundIdentifier{node.Identifier, pgsql.ColumnID}); also ensure each fragment's FROM includes the previous frame alias used in those expressions so the generated JOIN conditions like (s0.n1).id = e1.start_id are valid.
967-1004:⚠️ Potential issue | 🟠 MajorThe unconstrained step-expansion seed is still on the old node-joined shape.
Only the recursive leg is conditional now; the primer built just above still always joins the terminal node. That is why Line 45 in
cypher/models/pgsql/test/translation_cases/pattern_binding.sqland Lines 48 and 51 incypher/models/pgsql/test/translation_cases/pattern_expansion.sqlstill carryjoin node n2/n3on unconstrained[*..]legs. Please gate the primer’s right-node join the same way or the first hop keeps the extra filter/work.♻️ Suggested shape
- expansion.PrimerStatement.From = append(expansion.PrimerStatement.From, pgsql.FromClause{ - Source: pgsql.TableReference{ - Name: pgsql.CompoundIdentifier{traversalStep.Frame.Previous.Binding.Identifier}, - }, - Joins: []pgsql.Join{{ - Table: pgsql.TableReference{ - Name: pgsql.CompoundIdentifier{pgsql.TableEdge}, - Binding: models.OptionalValue(traversalStep.Edge.Identifier), - }, - JoinOperator: pgsql.JoinOperator{ - JoinType: pgsql.JoinTypeInner, - Constraint: expansionModel.EdgeJoinCondition, - }, - }, { - Table: pgsql.TableReference{ - Name: pgsql.CompoundIdentifier{pgsql.TableNode}, - Binding: models.OptionalValue(traversalStep.RightNode.Identifier), - }, - JoinOperator: pgsql.JoinOperator{ - JoinType: pgsql.JoinTypeInner, - Constraint: expansionModel.ExpansionNodeJoinCondition, - }, - }}, - }) + primerJoins := []pgsql.Join{{ + Table: pgsql.TableReference{ + Name: pgsql.CompoundIdentifier{pgsql.TableEdge}, + Binding: models.OptionalValue(traversalStep.Edge.Identifier), + }, + JoinOperator: pgsql.JoinOperator{ + JoinType: pgsql.JoinTypeInner, + Constraint: expansionModel.EdgeJoinCondition, + }, + }} + + if expansionModel.TerminalNodeConstraints != nil { + primerJoins = append(primerJoins, pgsql.Join{ + Table: pgsql.TableReference{ + Name: pgsql.CompoundIdentifier{pgsql.TableNode}, + Binding: models.OptionalValue(traversalStep.RightNode.Identifier), + }, + JoinOperator: pgsql.JoinOperator{ + JoinType: pgsql.JoinTypeInner, + Constraint: expansionModel.ExpansionNodeJoinCondition, + }, + }) + } + + expansion.PrimerStatement.From = append(expansion.PrimerStatement.From, pgsql.FromClause{ + Source: pgsql.TableReference{ + Name: pgsql.CompoundIdentifier{traversalStep.Frame.Previous.Binding.Identifier}, + }, + Joins: primerJoins, + })🤖 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 967 - 1004, The primer (seed) still always adds the right-node join even for unconstrained expansions; mirror the recursive leg's condition so the primer only adds the node join when expansionModel.TerminalNodeSatisfactionProjection != nil. Locate the primer/from construction (the code that builds the non-recursive seed, analogous to expansion.RecursiveStatement.From) and wrap the append of the pgsql.Join that uses traversalStep.RightNode.Identifier and expansionModel.ExpansionNodeJoinCondition with the same conditional used for recursiveJoins; ensure the join uses the same JoinOperator/Constraint shape as in the recursive case so unconstrained [*..] primers do not include the extra node join/filter.
🧹 Nitpick comments (4)
cypher/test/cases/positive_tests.json (1)
1-1: Consider keeping test fixture JSON pretty-printed for maintainability.Minifying the JSON to a single line makes diffs, code reviews, and manual edits significantly harder. For test fixtures where file size is rarely a concern, pretty-printed JSON improves readability and version control ergonomics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/test/cases/positive_tests.json` at line 1, The JSON fixture has been minified (the "test_cases" array is all on one line), hurting readability; reformat this file by pretty-printing the JSON with consistent indentation and line breaks (e.g., each test case object on its own block, keys like "name", "type", and "details" on separate lines) so diffs and reviews are easier to read—update the file containing the "test_cases" array accordingly and ensure the file remains valid JSON after formatting.cypher/models/pgsql/test/translation_integration_test.go (1)
68-70: Minor typo:cassesPassed→casesPassed.Suggested fix
var ( casesRun = 0 - cassesPassed = 0 + casesPassed = 0 )And update the reference at line 88:
if passed { - cassesPassed += 1 + casesPassed += 1 }And at line 95:
- fmt.Printf("Validated %d test cases with %d passing\n", casesRun, cassesPassed) + fmt.Printf("Validated %d test cases with %d passing\n", casesRun, casesPassed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/test/translation_integration_test.go` around lines 68 - 70, Rename the misspelled variable cassesPassed to casesPassed and update all references to it (e.g., where casesRun is initialized alongside it and any subsequent uses at/near the later assertions and increments) so the variable name is consistent (casesRun and casesPassed) throughout the test; ensure you update both its declaration and every read/write site to avoid undefined identifier errors.cypher/models/pgsql/test/assertions.go (1)
70-76:AssertAllstill re-buffers the full result for every built-in assertion.Each built-in helper immediately calls
newBufferedResult(result), soAssertAllbuffers once and then copies the in-memory rows again for every nested assertion. Reusing an incoming*bufferedResultwould make the advertised single-buffer behavior real.♻️ Possible shape
+func asBufferedResult(result graph.Result) *bufferedResult { + if br, ok := result.(*bufferedResult); ok { + br.Reset() + return br + } + return newBufferedResult(result) +}Then swap the existing
newBufferedResult(result)calls in the built-in assertions toasBufferedResult(result).Also applies to: 80-86, 90-96, 100-107, 113-122, 127-144, 148-157, 162-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/test/assertions.go` around lines 70 - 76, The built-in assertions (e.g., AssertEmpty) re-buffer the result by calling newBufferedResult(result) each time; update these helpers to use an asBufferedResult(result) conversion so they reuse an existing *bufferedResult when present (making AssertAll's single-buffer promise real). Locate all assertions that call newBufferedResult (AssertEmpty and the other built-ins referenced) and replace those calls with asBufferedResult(result), ensuring any code that expects a concrete *bufferedResult type accepts/handles it accordingly and that newBufferedResult remains for creating a fresh buffer only when needed; keep the function names newBufferedResult, asBufferedResult, AssertAll, and bufferedResult as the anchors for your changes.cypher/models/pgsql/test/fixture.go (1)
21-31: Avoid hand-syncingSemanticGraphSchemain two places.The comment already says this definition must match
validation_integration_test.go. Keeping two ordered schema copies invites kind-ID drift and brittle golden SQL updates; pull this into one shared declaration/helper instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/test/fixture.go` around lines 21 - 31, The SemanticGraphSchema variable is duplicated and must be centralized to avoid kind-ID drift: remove the hand-copied SemanticGraphSchema declaration here and instead import or reference the single shared schema provider used by validation_integration_test.go (or move the canonical graph.Schema into a shared helper), ensuring the same graph.Schema value (containing Graphs with Name "test", Nodes: NodeKind1/NodeKind2, Edges: EdgeKind1/EdgeKind2, and DefaultGraph Name "test") is returned from that shared function/variable; update any tests that used the local SemanticGraphSchema to call the shared helper (keeping graph.Schema, SemanticGraphSchema identifier usages consistent) so kind IDs remain in one authoritative place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 54-61: The Makefile's clean target must also remove the PostgreSQL
update staging directory created by test_update; update the clean target to
delete cypher/models/pgsql/test/updated_cases (in addition to
cypher/analyzer/updated_cases/) so failed update runs can’t leave stale
fixtures. Locate the Makefile targets test_update and clean and add an rm -rf
for cypher/models/pgsql/test/updated_cases (and mirror any trailing slash
variants) to the clean recipe.
- Around line 42-45: The test_bench Makefile target currently runs `go test`
with `-tags benchmarks` but omits the `-bench` flag so Benchmark* functions
won't run; update the `test_bench` recipe (target name `test_bench`) to add
`-bench=.` to the `$(GO_CMD) test` invocation (keeping existing flags like
`-tags benchmarks`, `-race`, `-cover`, `-count=1`, `-parallel=1` and
`$(MAIN_PACKAGES)`) so all benchmark functions are executed when the target is
invoked.
---
Outside diff comments:
In `@cypher/models/pgsql/translate/projection.go`:
- Around line 526-550: The code assumes a 1:1 mapping between
currentPart.projections.Items and the flattened projection slice (projection),
which breaks when buildExternalProjection expands a single input item into
multiple output items (e.g., buildProjectionForExpansionNode /
buildProjectionForNodeComposite producing a flat _id). Fix by changing
buildExternalProjection to also return a mapping from each input projection item
index to the list/range of indices in the flattened projection slice (e.g.,
projectionIndexMap map[int][]int or []int offsets per item), then update this
loop to iterate currentPart.projections.Items and for each item use the mapping
to collect all corresponding projExpr entries (checking each mapped index and
appending aliasedExpr.Expression or projExpr to nonAggregateExprs as before)
before assigning singlePartQuerySelect.GroupBy; reference
buildExternalProjection, buildProjectionForExpansionNode,
buildProjectionForNodeComposite, currentPart.projections.Items, projection, and
singlePartQuerySelect.GroupBy.
In `@drivers/neo4j/batch_integration_test.go`:
- Line 64: Fix the typo in the comment string "Regsiter a cleanup step to wipe
the database" in drivers/neo4j/batch_integration_test.go by changing "Regsiter"
to "Register" so the comment reads "Register a cleanup step to wipe the
database"; locate the comment (exact text shown) and update it accordingly.
---
Duplicate comments:
In `@cardinality/hyperloglog_bench_test.go`:
- Around line 365-368: The deviation computation uses a signed formula so large
underestimates produce negative values that can incorrectly pass the threshold;
update the calculation to use absolute relative error (e.g. compute deviation as
the absolute percentage difference between estimatedCardinality (from
sketch.Cardinality()) and cardinalityMax: deviation =
math.Abs(float64(estimatedCardinality)-cardinalityMax)/cardinalityMax*100) and
replace the current deviation formula wherever used (references:
estimatedCardinality, deviation, sketch.Cardinality(), cardinalityMax) so the
comparisons against 0.5 / 0.66 thresholds correctly use the absolute error.
In `@cypher/models/pgsql/test/cases.go`:
- Around line 1710-1727: The PostAssert for the "detach-delete a typed node and
its incident edges" test (PostAssert function using tx.Query and iterating
result.Next()) doesn't verify the fixture row was removed; update the fixture
creation (NewNodeWithProperties for "victim", NodeKind1) to include a unique
sentinel property (e.g. "sentinel": "unique-<something>") and change the
PostAssert to run the same query filtering on that sentinel, then assert that
result.Next() is false (and fail the test via t.Fatalf or t.Errorf if it returns
true), while still checking result.Error() after closing the result.
In `@cypher/models/pgsql/translate/expansion.go`:
- Around line 14-25: The shortest-path harness fragments generated by
prepareForwardFrontPrimerQuery and prepareForwardFrontRecursiveQuery still
reference prior-frame node IDs without adding the prior frame to the fragment’s
FROM or applying the flat-ID rewrite; update both functions so they build their
`@pi0/`@pi1 fragments using the same plumbing as nodeIDRef: when the
BoundIdentifier.HasFlatID is true emit a pgsql.CompoundIdentifier of the prior
frame (frameID, Identifier + "_id"), otherwise call
rewriteCompositeTypeFieldReference(frameID,
pgsql.CompoundIdentifier{node.Identifier, pgsql.ColumnID}); also ensure each
fragment's FROM includes the previous frame alias used in those expressions so
the generated JOIN conditions like (s0.n1).id = e1.start_id are valid.
- Around line 967-1004: The primer (seed) still always adds the right-node join
even for unconstrained expansions; mirror the recursive leg's condition so the
primer only adds the node join when
expansionModel.TerminalNodeSatisfactionProjection != nil. Locate the primer/from
construction (the code that builds the non-recursive seed, analogous to
expansion.RecursiveStatement.From) and wrap the append of the pgsql.Join that
uses traversalStep.RightNode.Identifier and
expansionModel.ExpansionNodeJoinCondition with the same conditional used for
recursiveJoins; ensure the join uses the same JoinOperator/Constraint shape as
in the recursive case so unconstrained [*..] primers do not include the extra
node join/filter.
---
Nitpick comments:
In `@cypher/models/pgsql/test/assertions.go`:
- Around line 70-76: The built-in assertions (e.g., AssertEmpty) re-buffer the
result by calling newBufferedResult(result) each time; update these helpers to
use an asBufferedResult(result) conversion so they reuse an existing
*bufferedResult when present (making AssertAll's single-buffer promise real).
Locate all assertions that call newBufferedResult (AssertEmpty and the other
built-ins referenced) and replace those calls with asBufferedResult(result),
ensuring any code that expects a concrete *bufferedResult type accepts/handles
it accordingly and that newBufferedResult remains for creating a fresh buffer
only when needed; keep the function names newBufferedResult, asBufferedResult,
AssertAll, and bufferedResult as the anchors for your changes.
In `@cypher/models/pgsql/test/fixture.go`:
- Around line 21-31: The SemanticGraphSchema variable is duplicated and must be
centralized to avoid kind-ID drift: remove the hand-copied SemanticGraphSchema
declaration here and instead import or reference the single shared schema
provider used by validation_integration_test.go (or move the canonical
graph.Schema into a shared helper), ensuring the same graph.Schema value
(containing Graphs with Name "test", Nodes: NodeKind1/NodeKind2, Edges:
EdgeKind1/EdgeKind2, and DefaultGraph Name "test") is returned from that shared
function/variable; update any tests that used the local SemanticGraphSchema to
call the shared helper (keeping graph.Schema, SemanticGraphSchema identifier
usages consistent) so kind IDs remain in one authoritative place.
In `@cypher/models/pgsql/test/translation_integration_test.go`:
- Around line 68-70: Rename the misspelled variable cassesPassed to casesPassed
and update all references to it (e.g., where casesRun is initialized alongside
it and any subsequent uses at/near the later assertions and increments) so the
variable name is consistent (casesRun and casesPassed) throughout the test;
ensure you update both its declaration and every read/write site to avoid
undefined identifier errors.
In `@cypher/test/cases/positive_tests.json`:
- Line 1: The JSON fixture has been minified (the "test_cases" array is all on
one line), hurting readability; reformat this file by pretty-printing the JSON
with consistent indentation and line breaks (e.g., each test case object on its
own block, keys like "name", "type", and "details" on separate lines) so diffs
and reviews are easier to read—update the file containing the "test_cases" array
accordingly and ensure the file remains valid JSON after formatting.
🪄 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: 68a0a0ad-a490-4596-aa8c-51b02aeb085a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
MakefileREADME.mdcardinality/hyperloglog32_test.gocardinality/hyperloglog64_test.gocardinality/hyperloglog_bench_test.gocypher/models/pgsql/operators.gocypher/models/pgsql/test/assertions.gocypher/models/pgsql/test/cases.gocypher/models/pgsql/test/fixture.gocypher/models/pgsql/test/semantic_integration_test.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/parameters.sqlcypher/models/pgsql/test/translation_cases/pattern_binding.sqlcypher/models/pgsql/test/translation_cases/pattern_expansion.sqlcypher/models/pgsql/test/translation_cases/pattern_rewriting.sqlcypher/models/pgsql/test/translation_cases/quantifiers.sqlcypher/models/pgsql/test/translation_cases/scalar_aggregation.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/translation_integration_test.gocypher/models/pgsql/test/translation_test.gocypher/models/pgsql/translate/expansion.gocypher/models/pgsql/translate/match.gocypher/models/pgsql/translate/projection.gocypher/models/pgsql/translate/renamer.gocypher/models/pgsql/translate/tracking.gocypher/models/pgsql/translate/with.gocypher/test/cases/filtering_tests.jsoncypher/test/cases/mutation_tests.jsoncypher/test/cases/negative_tests.jsoncypher/test/cases/positive_tests.jsondrivers/neo4j/batch_integration_test.gogo.mod
💤 Files with no reviewable changes (4)
- go.mod
- cardinality/hyperloglog32_test.go
- cardinality/hyperloglog64_test.go
- cypher/models/pgsql/test/translation_test.go
…bility - the pg query planner is blind to composite types in certain situations
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.
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests
Chores