Replace parallel condition/result vectors with single CaseWhen vector in Expr::Case #1733
Conversation
This file contains 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
lovasoa
force-pushed
the
case_representation
branch
2 times, most recently
from
0503160 to
afd8e6e
Compare
|
fixes sqlpage/SQLPage#818 |
lovasoa
force-pushed
the
case_representation
branch
from
afd8e6e to
6b00c3b
Compare
… in Expr::Case The primary motivation for this change is to fix the visitor traversal order for CASE expressions. In SQL, CASE expressions follow a specific syntactic order (e.g., `CASE a WHEN 1 THEN 2 WHEN 3 THEN 4 ELSE 5`), AST visitors now process nodes in the same order as they appear in the source code. The previous implementation, using separate `conditions` and `results` vectors, would visit all conditions first and then all results, which didn't match the source order. The new `CaseWhen` structure ensures visitors process expressions in the correct order: `a,1,2,3,4,5`. A secondary benefit is making invalid states unrepresentable in the type system. The previous implementation using parallel vectors (`conditions` and `results`) made it possible to create invalid CASE expressions where the number of conditions didn't match the number of results. When this happened, the `Display` implementation would silently drop elements from the longer list, potentially masking bugs. The new `CaseWhen` struct couples each condition with its result, making it impossible to create such mismatched states. While this is a breaking change to the AST structure, sqlparser has a history of making such changes when they improve correctness. I don't expect significant downstream breakages, and the benefits of correct visitor ordering and type safety are significant, so I think the trade-off is worthwhile.
lovasoa
force-pushed
the
case_representation
branch
from
6b00c3b to
04732cf
Compare
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.
The primary motivation for this change is to fix the visitor traversal order for CASE expressions. In SQL, CASE expressions follow a specific syntactic order (e.g.,
CASE a WHEN 1 THEN 2 WHEN 3 THEN 4 ELSE 5), AST visitors now process nodes in the same order as they appear in the source code. The previous implementation, using separateconditionsandresultsvectors, would visit all conditions first and then all results, which didn't match the source order. The newCaseWhenstructure ensures visitors process expressions in the correct order:a,1,2,3,4,5.A secondary benefit is making invalid states unrepresentable in the type system. The previous implementation using parallel vectors (
conditionsandresults) made it possible to create invalid CASE expressions where the number of conditions didn't match the number of results. When this happened, theDisplayimplementation would silently drop elements from the longer list, potentially masking bugs. The newCaseWhenstruct couples each condition with its result, making it impossible to create such mismatched states.While this is a breaking change to the AST structure, sqlparser has a history of making such changes when they improve correctness. I don't expect significant downstream breakages, and the benefits of correct visitor ordering and type safety are significant, so I think the trade-off is worthwhile.