Fix issue #24701 Better error when the query contains reserved names - #24715
Open
Ruchirtripathi wants to merge 4 commits into
Open
Fix issue #24701 Better error when the query contains reserved names #24715Ruchirtripathi wants to merge 4 commits into
Ruchirtripathi wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24715 +/- ##
=======================================
Coverage 81.51% 81.51%
=======================================
Files 1123 1123
Lines 405139 405151 +12
Branches 405139 405151 +12
=======================================
+ Hits 330242 330263 +21
+ Misses 55568 55560 -8
+ Partials 19329 19328 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
@Ruchirtripathi Thank you for the fix! Is it possible to add a test to show the new error message? |
Contributor
Author
|
@appletreeisyellow Thank you for the review! I've just pushed a commit that adds a test for this in sql_integration.rs. It verifies that attempting to alias a column to __common_expr_1 correctly throws the reserved name error. |
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.
Which issue does this PR close?
• Closes #24701
Rationale for this change
When a query renames a column using a DataFusion reserved name (specifically names starting with __common_expr, which are used internally by the Common
Subexpression Elimination optimizer pass), it currently fails during execution with obscure Arrow errors (e.g., Arrow error: Invalid argument error:
Invalid comparison operation: Float64 > Boolean). This happens because the user-provided alias collides with the internal optimizer logic.
This change ensures we fail earlier and return a clear, descriptive planning error to the user indicating that the column name is reserved.
What changes are included in this PR?
• Added a check_plan validation step inside SqlToRel::sql_statement_to_plan_with_context in datafusion/sql/src/statement.rs.
• The unoptimized LogicalPlan is traversed, and if any field in any node's schema starts with the reserved prefix __common_expr, we immediately throw a
plan_err!.
• Doing this validation on the raw unoptimized plan ensures that we correctly flag user-provided reserved aliases without incorrectly flagging the
__common_expr aliases legitimately added later by the CSE optimizer pass.
Are these changes tested?
Yes, they have been verified against the reproducer provided in the issue to return the correct error (__common_expr_2 is a reserved DataFusion column
name, please use another name). It was also verified that this does not break existing logic where CSE natively inserts these aliases (e.g.,
list_view_agg_test passes successfully).
Are there any user-facing changes?
Yes. Users who alias their columns with the __common_expr prefix will now receive a clear, actionable planning error instead of a confusing Arrow
execution error.