You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Ruchirtripathi (via GitHub)" <gi...@apache.org> on 2026/08/27 07:29:36 UTC
[PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Ruchirtripathi opened a new pull request, #24715:
URL: https://github.com/apache/datafusion/pull/24715
## 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #24715:
URL: https://github.com/apache/datafusion/pull/24715
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24715:
URL: https://github.com/apache/datafusion/pull/24715#discussion_r3903657028
##########
datafusion/sql/src/statement.rs:
##########
Review Comment:
It looks like `check_plan` is only reached for the `DFStatement::Statement` branch here. `DFStatement::CopyTo` goes directly through `copy_to_plan`, and its query input is built with `query_to_plan`, so a query such as `COPY (SELECT 1 AS __common_expr_1) TO 'output.csv' STORED AS CSV` can still reach optimization with the reserved name. `EXPLAIN COPY` appears to follow the same path recursively through `statement_to_plan`.
Could we validate the completed plan at the `statement_to_plan` boundary after all `DFStatement` variants have been planned? I think we should still keep validation for public `sql_statement_to_plan` callers as well. It would also be good to add a regression test covering the `COPY` query case.
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -5813,3 +5813,13 @@ impl HigherOrderUDFImpl for MockArrayReduce {
unreachable!()
}
}
+
+#[test]
+fn test_reserved_column_name() {
Review Comment:
Small non-blocking suggestion: could we also add a boundary test showing that a name which only contains the token, such as `user__common_expr_1`, is still accepted? That would document the intended `starts_with` behavior and help prevent this check from accidentally becoming broader in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24715:
URL: https://github.com/apache/datafusion/pull/24715#issuecomment-5617626451
Thank y ou @Ruchirtripathi and @kosiew
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24715:
URL: https://github.com/apache/datafusion/pull/24715#issuecomment-5616654171
🚀
@Ruchirtripathi
Thank you for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Posted by "Ruchirtripathi (via GitHub)" <gi...@apache.org>.
Ruchirtripathi commented on PR #24715:
URL: https://github.com/apache/datafusion/pull/24715#issuecomment-5461018100
@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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24715:
URL: https://github.com/apache/datafusion/pull/24715#discussion_r3978376698
##########
datafusion/sql/src/statement.rs:
##########
Review Comment:
Would it be possible to look into putting this check in the optimizer that has the requirement instead of the SQL planner, as suggested here https://github.com/apache/datafusion/issues/24701#issuecomment-5428054288 ?
There is no reason in general Datafusion can't plan such queries, it is only if that optimizer pass is invoked (so I think the error should live in the optimizer pass)
Also, can you please add .slt tests (not just unit tests)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Posted by "Ruchirtripathi (via GitHub)" <gi...@apache.org>.
Ruchirtripathi commented on code in PR #24715:
URL: https://github.com/apache/datafusion/pull/24715#discussion_r3964700464
##########
datafusion/sql/src/statement.rs:
##########
Review Comment:
Thanks for catching this! @kosiew I've updated statement_to_plan to validate the constructed LogicalPlan at the boundary as
suggested, ensuring that both COPY and EXPLAIN COPY paths are now properly checked. I also kept the existing
validation in sql_statement_to_plan_with_context for public callers.
Additionally, I've added the regression test for the COPY case, along with a boundary test to ensure that valid identifiers like user__common_expr_1 are still accepted. Let me know if this looks good to you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Posted by "Ruchirtripathi (via GitHub)" <gi...@apache.org>.
Ruchirtripathi commented on PR #24715:
URL: https://github.com/apache/datafusion/pull/24715#issuecomment-5612820876
@kosiew can you go ahead with the merge or you need something else?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Posted by "Ruchirtripathi (via GitHub)" <gi...@apache.org>.
Ruchirtripathi commented on code in PR #24715:
URL: https://github.com/apache/datafusion/pull/24715#discussion_r3964705821
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -5813,3 +5813,13 @@ impl HigherOrderUDFImpl for MockArrayReduce {
unreachable!()
}
}
+
+#[test]
+fn test_reserved_column_name() {
Review Comment:
Great suggestion! I've added a boundary test for user__common_expr_1 to explicitly document the starts_with behavior and ensure it doesn't accidentally get broadened in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24715:
URL: https://github.com/apache/datafusion/pull/24715#issuecomment-5436239081
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24715?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `80.00000%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.46%. Comparing base ([`574fe67`](https://app.codecov.io/gh/apache/datafusion/commit/574fe67f9add81155dd440acee64547cd9f49082?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`bbf9a54`](https://app.codecov.io/gh/apache/datafusion/commit/bbf9a54cbf3420867e8dcb27a320c71307c1d027?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 165 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24715?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/sql/src/statement.rs](https://app.codecov.io/gh/apache/datafusion/pull/24715?src=pr&el=tree&filepath=datafusion%2Fsql%2Fsrc%2Fstatement.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zcWwvc3JjL3N0YXRlbWVudC5ycw==) | 80.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24715?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24715 +/- ##
==========================================
+ Coverage 81.17% 81.46% +0.28%
==========================================
Files 1109 1120 +11
Lines 388164 401301 +13137
Branches 388164 401301 +13137
==========================================
+ Hits 315109 326903 +11794
- Misses 54509 55273 +764
- Partials 18546 19125 +579
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24715?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix issue #24701 Better error when the query contains reserved names [datafusion]
Posted by "appletreeisyellow (via GitHub)" <gi...@apache.org>.
appletreeisyellow commented on PR #24715:
URL: https://github.com/apache/datafusion/pull/24715#issuecomment-5455958833
@Ruchirtripathi Thank you for the fix! Is it possible to add a test to show the new error message?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org