fix: VLP WITH clause property resolution and path_relationships array#206
Merged
fix: VLP WITH clause property resolution and path_relationships array#206
Conversation
Three bugs fixed in VLP (variable-length path) + WITH clause queries: 1. VLP property requirements matching: PropertyRequirementsAnalyzer records DB column names (e.g., "full_name") after view resolution, but VLP CTE property pruning only checked Cypher property names (e.g., "name"). Fix: check both names in the pruning loop (cte_extraction.rs). 2. DB-to-Cypher property name translation: When WITH uses specific property access (e.g., `WITH u1.name as start_name`), the CTE body generated `t.start_full_name` instead of `t.start_name`. Fix: pre-translate DB column names to Cypher property names before VLP rewriting (plan_builder_utils.rs). 3. path_relationships UInt8 placeholder: VLP CTEs used `CAST(0 AS UInt8)` for path_relationships because `plan_uses_relationships_fn` couldn't detect `relationships(path)` calls in parent WITH clauses (root_plan only covered the VLP subtree). Fix: always generate proper relationship arrays when a path variable is bound (variable_length_cte.rs). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 tasks
genezhang
added a commit
that referenced
this pull request
Mar 11, 2026
Remove xfail from 5 tests now passing after JOIN rewriting fix: - test_vlp_with_count_distinct_basic - test_vlp_with_multiple_aggregates - test_vlp_with_order_by_aggregate - test_vlp_with_nested_property_aggregate - test_vlp_only_vlp_nodes_in_aggregate Update remaining xfail reasons with specific root causes: - test_vlp_with_sum_aggregate: needs VLP property columns (PR #206) - test_vlp_no_aggregation: needs VLP property columns (PR #206) - test_vlp_bidirectional_with_aggregate: CTE column resolution - test_vlp_with_having_equivalent: CTE column resolution - test_vlp_different_hop_counts: *1..3 timeout on test data Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes variable-length path (VLP) SQL generation edge cases around WITH-clause property pruning/name resolution and ensures path_relationships is emitted as an array whenever a path variable is bound (to support relationships(path) usage above the VLP subtree).
Changes:
- Fix VLP property pruning to match both Cypher property names and resolved DB column names.
- Pre-translate resolved DB column names back to Cypher property names for VLP WITH CTE body rewriting.
- Treat bound path variables as always requiring
path_relationshipsarray data.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/rust/integration/complex_feature_tests.rs | Adds regression tests covering VLP WITH specific property access and path_relationships non-UInt8 placeholder behavior. |
| src/render_plan/plan_builder_utils.rs | Adds helper to translate DB column names back to Cypher property names before VLP expression rewriting in WITH CTE bodies. |
| src/render_plan/cte_extraction.rs | Updates VLP property-pruning logic to check required properties against both Cypher and DB column identifiers. |
| src/clickhouse_query_generator/variable_length_cte.rs | Changes path-data requirement logic so bound path variables always trigger path_relationships array population. |
Open issues to address before merge:
src/render_plan/plan_builder_utils.rs: The doc comment forrewrite_render_expr_for_vlp_with_from_aliasmentions adb_to_cypherparameter/mapping, but the function signature doesn’t take it; this is misleading documentation and should be updated to reflect the current design (translation happens via a separate pre-pass).tests/rust/integration/complex_feature_tests.rs:test_vlp_path_relationships_is_array’s doc comment says “even without explicit relationships() call” but the query does callrelationships(path). Also, the assertion matches an exact, case-sensitive SQL substring, which is more brittle than the surrounding tests’to_lowercase()-based checks.src/clickhouse_query_generator/variable_length_cte.rs:needs_path_data()no longer consultsneeds_path_relationships, making that flag (and its associated comments/plumbing elsewhere) effectively unused/misleading; either remove the flag and update docs, or keep the flag and apply it consistently.
…aversal, type consistency 1. Normalize SQL to lowercase before checking for UInt8 placeholder in test 2. Fix doc comment to match test query (uses relationships(path) in WITH) 3. Update rewrite_render_expr_for_vlp_with_from_alias doc to reflect that db_to_cypher translation now happens before this function is called 4. Add recursion into InSubquery, MapLiteral, ArraySubscript, ArraySlicing, and ReduceExpr variants in translate_db_columns_to_cypher_properties 5. Change CAST(0 AS UInt8) to CAST([] AS Array(String)) for path_relationships in BFS and weighted VLP branches for type consistency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
genezhang
added a commit
that referenced
this pull request
Mar 12, 2026
Remove @pytest.mark.xfail from tests that pass after recent VLP, shortestPath, denormalized schema, and multi-type property fixes (PRs #206-#210). Affected areas: VLP aggregation, shortest paths, path variables, edge constraints, multi-type property extraction, denormalized VLP, VLP relationship return, VLP WITH comprehension, pattern matrix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 tasks
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
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
Details
Bug 1 — Property pruning mismatch:
PropertyRequirementsAnalyzerrecords DB column names (full_name) after view resolution, but VLP CTE property pruning incte_extraction.rsonly checked Cypher property names (name). Result: VLP CTE had zero property columns forWITH u1.name as start_namequeries. Fix: check bothprop_name(Cypher) andprop_value.raw()(DB) in the pruning loop.Bug 2 — Column name translation: When WITH uses specific property access (
WITH u1.name as start_name), the CTE body generatedt.start_full_name(using DB column name) instead oft.start_name(using Cypher property name that VLP CTE columns use). Fix: addedtranslate_db_columns_to_cypher_properties()helper that pre-translates PropertyAccessExp column names using CTE metadata before VLP rewriting.Bug 3 — UInt8 path_relationships: VLP CTEs used
CAST(0 AS UInt8)as placeholder forpath_relationshipsbecauseplan_uses_relationships_fncheckedcontext.root_planwhich only contained the VLP subtree — it couldn't seerelationships(path)calls in parent WITH clauses. Fix:needs_path_data()now returns true whenever a path variable is bound, since the overhead of tracking a relationship type array is minimal.Test plan
test_vlp_with_specific_property_access,test_vlp_path_relationships_is_arraycargo fmt --all && cargo clippy --all-targetsclean🤖 Generated with Claude Code