fix: VLP aggregation JOIN rewriting and start/end swap for UNION branches#207
Merged
fix: VLP aggregation JOIN rewriting and start/end swap for UNION branches#207
Conversation
…ches Three fixes for VLP queries with post-VLP relationships and aggregations: 1. Rewrite JOIN conditions in VLP UNION branches: JOIN ON conditions referenced original Cypher aliases (e.g., u2.user_id) instead of VLP CTE columns (e.g., t.end_id). Added JOIN condition rewriting in rewrite_vlp_branch_select() and dependency-based JOIN sorting. 2. Swap start/end columns in reverse UNION branch inner SELECT: For aggregation+UNION queries, the inner SELECT was shared across all branches. Reverse branches need t.start_id where forward uses t.end_id. Added per-branch direction detection and swap_vlp_start_end(). 3. Include VLP column references in aggregate argument extraction: collect_property_access_sql() now handles Column and TableAlias expressions (produced by VLP rewriting), ensuring aggregate arguments like COUNT(DISTINCT t.end_id) are included in inner UNION SELECT. Fixes 6 of 10 VLP aggregation xfail tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
Fixes SQL rendering for variable-length path (VLP) queries that combine UNION branches with aggregation, ensuring JOIN conditions and aggregate inputs are correctly rewritten against VLP CTE output columns.
Changes:
- Rewrite JOIN
ON/pre_filterexpressions inside VLP UNION branches to reference VLP CTE columns (e.g.,t.end_id) instead of original Cypher aliases. - Improve aggregate argument column collection for UNION+aggregation so outer aggregates can reference inner projections after VLP rewriting.
- Update integration tests by removing/adjusting
xfailmarkers to reflect newly passing cases and remaining known limitations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/clickhouse_query_generator/to_sql_query.rs |
Adds VLP UNION-branch JOIN rewriting, aggregate arg extraction updates, and start/end swap logic for reverse UNION branches under aggregation. |
tests/integration/test_vlp_aggregation.py |
Updates xfail usage/reasons for VLP+aggregation integration tests based on the fixes and known remaining gaps. |
…t removal Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
-[:AUTHORED]->(p:Post)after VLP) now correctly reference VLP CTE columns instead of original Cypher aliasest.start_id/t.end_idare now correctly swapped per branch directionTest plan
🤖 Generated with Claude Code