Skip to content

fix: denormalized VLP cycle prevention skips table lookup for virtual nodes#231

Merged
genezhang merged 1 commit intomainfrom
fix/denormalized-vlp-cycle-prevention
Mar 14, 2026
Merged

fix: denormalized VLP cycle prevention skips table lookup for virtual nodes#231
genezhang merged 1 commit intomainfrom
fix/denormalized-vlp-cycle-prevention

Conversation

@genezhang
Copy link
Copy Markdown
Owner

Summary

  • Fixed-length VLP (*2, *3) on denormalized schemas failed with "Missing table information for start node in cycle prevention" because extract_table_name returns None for virtual nodes
  • Fix: move extract_table_name calls inside the non-denormalized branch — denormalized patterns use from_id/to_id columns directly for cycle prevention
  • Applied to both filter_builder.rs and plan_builder_utils.rs (parallel implementations)

Impact

  • 19 xfails promoted to passing (9 denormalized + 10 polymorphic VLP/shortest_path tests)
  • 3 tests converted from xfail to skip (invalid a.airport property)
  • 12 coupled schema xfails remain (multi-relationship VLP on shared tables — untestable without ClickHouse)

Updated test counts

  • Before: 132 passed, 35 skipped, 34 xfailed
  • After: 151 passed, 38 skipped, 12 xfailed

Test plan

  • All 1588 Rust tests pass
  • cargo fmt and cargo clippy clean
  • SQL generation verified for all denormalized VLP patterns (exact, range, path_var, shortest_path)
  • Polymorphic VLP SQL generation verified
  • Integration tests with ClickHouse (unavailable during development)

🤖 Generated with Claude Code

… nodes

Fixed-length VLP (*2, *3) on denormalized schemas failed with "Missing
table information for start node in cycle prevention" because
extract_table_name returns None for virtual nodes that have no separate
table.

Fix: move extract_table_name calls inside the non-denormalized branch
in both filter_builder.rs and plan_builder_utils.rs. For denormalized
patterns, cycle prevention uses relationship columns (from_id/to_id)
directly — no table name needed.

Promotes 19 xfails:
- 9 denormalized VLP tests (vlp_exact, vlp_range, vlp_path_var, shortest_path)
- 10 polymorphic VLP tests (already generating correct SQL)
Converts 3 invalid-property tests to skips (a.airport not valid).
12 coupled schema xfails remain (multi-relationship VLP untestable).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@genezhang
Copy link
Copy Markdown
Owner Author

Code Review

Fix is clean and correct

The root cause is clear: extract_table_name returns None for denormalized virtual nodes (they don't have a separate table), causing the cycle prevention code to error out with "Missing table information for start node." Moving the extract_table_name calls inside the else (non-denormalized) branch is the right fix — denormalized patterns use from_id/to_id relationship columns directly and never need the table names.

Pre-existing asymmetry between parallel implementations

The two parallel implementations have a subtle difference in the non-denormalized branch that predates this PR but is worth noting:

  • filter_builder.rs uses extract_id_column() for start/end node ID resolution
  • plan_builder_utils.rs uses extract_end_node_id_column() with a comment explaining it handles nested GraphRel patterns

This divergence isn't introduced by this PR, but the two files are clearly meant to be parallel implementations. If one is correct for nested GraphRel, the other probably should be too.

Test changes

  • 19 xfail→passing: good progress (9 denormalized VLP + 10 polymorphic VLP/shortest_path)
  • 3 xfail→skip: Airport airport property tests — consistent with PR 227's skip reasons
  • 12 coupled schema xfails updated with more precise reason ("VLP on multi-relationship denormalized tables" vs the old generic "not supported") — nice improvement in documentation

Result: 151 passed, 38 skipped, 12 xfailed — solid improvement

LGTM

@genezhang genezhang merged commit 7ead37f into main Mar 14, 2026
4 checks passed
@genezhang genezhang deleted the fix/denormalized-vlp-cycle-prevention branch March 14, 2026 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant