-
Notifications
You must be signed in to change notification settings - Fork 112
Improved documentation of CTEs and documentation validation #3637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- add tests for preorder / level traversal in YAML.
📊 Metrics Diff Analysis ReportSummary
ℹ️ About this analysisThis automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:
The last category in particular may indicate planner regressions that should be investigated. New QueriesCount of new queries by file:
Plan and Metrics ChangedThese queries experienced both plan and metrics changes. This generally indicates that there was some planner change Total: 3 queries Statistical Summary (Plan and Metrics Changed)
There were no queries with significant regressions detected. Minor Changes (Plan and Metrics Changed)In addition, there were 3 queries with minor changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent documentation!
Apologies for extra comments not focusing on the documentation commit, I only noticed your comment about which commit to review after the fact.
Assertions.assertThat(baseVisitorCalled.booleanValue()).isTrue(); | ||
} | ||
|
||
@ParameterizedTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure I understand what that test does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ensures the DelegatingVisitor
is invoked during traversal order parsing to address a test coverage gap identified by TeamScale.
The uncovered code stems from the query parsing mechanism: when parsing recursive CTEs (rCTEs) with traversal orders, the parser bypasses the delegating visitor and directly examines the inner traversal context within the same method. Consequently, TeamScale flags the traversal strategy implementation in DelegatingVisitor
as unused. However, since the root interface RelationalParserVisitor
is auto-generated by ANTLR, we must implement these methods despite their limited practical usage.
; | ||
|
||
traversalStrategy | ||
: PREORDER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if one is called PRE-ORDER, the other one should be called LEVEL-ORDER.
Or you could have: TRAVERSAL ORDER PRE / TRAVERSAL ORDER LEVEL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with adapting the traversal keywords as long as we do not introduce a hyphen, or use an underscore instead, hyphen is somewhat awkward and might not be aligned with standard SQL keywords, and major vendors as far as I know.
Terminal('TRAVERSAL'), | ||
Terminal('='), | ||
Choice(0, | ||
Terminal('ANY'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need an ANY
, or could we assume that the absence of a specified traversal order is ANY
, a little like the absence of an index hint means any index is fair game.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, ANY
is probably not needed.
Terminal('='), | ||
Choice(0, | ||
Terminal('ANY'), | ||
Terminal('PREORDER'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would suggest a parenthesis-less syntax:
WITH RECURSIVE cte
TRAVERSAL ORDER PRE-ORDER AS (
SELECT ...
)
SELECT * FROM cte
Or
WITH RECURSIVE cte AS (
SELECT ...
UNION ALL
SELECT ...
)
TRAVERSAL ORDER LEVEL-ORDER
SELECT * FROM cte
with a significant preference for the second proposal since it aligns with the SEARCH clause in terms of positioning in the query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Binding the syntax closer to recursive
makes sense considering that traversal is only meaningful with rCTE, however I also like the second syntax better, but without the hyphen:
TRAVERSAL ORDER PRE_ORDER
and
TRAVERSAL ORDER LEVEL_ORDER
This PR significantly improves the documentation for recursive Common Table Expressions, providing users with comprehensive guidance on
leveraging this powerful SQL feature.
Documentation Improvements
TRAVERSAL
clause options (PREORDER
,LEVEL
,ANY
)Documentation Validation
implementation
drift
This approach ensures that users always have access to accurate, working examples while maintaining long-term documentation quality through automated testing.
This solves #3636.