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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
