davisp opened a new pull request, #13632: URL: https://github.com/apache/datafusion/pull/13632
Previously, a query like `SELECT $1;` would fail to generate a LogicalPlan. With this change these queries are now workable. This required creating a new LogicalPlan::validate_parametere_types that is called from Optimizer::optimize to assert that all parameter types have been inferred. ## Which issue does this PR close? Closes #5617 This might also close an untracked step from #4539 which is listed as "Support PREPARE statements without explicit parameters". The tests I updated mentioned a `TODO` for supporting placeholders without types which is now shown to be fixed in this PR. But I'm uncertain enough on the listed step in that epic to pre-emptively create an issue to attach to it. If this does cover it, I would be more than happy to open the issue and add a comment to the epic as well as updating this PR description. ## Rationale for this change Initially I just wanted to support `SELECT $1;` in a FlightSQL server. It appears fixing that has also fixed a number of other issues. ## What changes are included in this PR? This removes the error from `ExprSchemable::get_type` when a Placeholder has no specified type. I believe this error to be a bug based on the fact that its suggesting that the error can be fixed by providing parameter values and the fact that the error is triggered when creating a LogicalPlan. I've instead added a new `LogicalPlan::validate_placeholder_types` method which is invoked at the start of the Optimizer::optimize method. I should note, that this PR may also be related to #8819. While developing this, I noticed that the Optimizer seems perfectly happy to optimize plans where some datatypes are `DataType::Null`. I have absolutely nowhere near enough context to know if that's something that should be allowed or not so I added the `validate_placeholder_types` so that the old behavior is maintained where optimization fails on untyped placeholders. ## Are these changes tested? Yep. I've updated/removed old tests that asserted the broken behavior and added tests that cover both my case of `SELECT $1;` as well as `SELECT * FROM foo WHERE col LIKE $1` which covers #5617. ## Are there any user-facing changes? Some queries that would have failed planning may now fail at optimization time instead? Not sure if that's important. Also, because I was moving the error message anyway, I included the Placeholder.id in the error text which seemed like it'd help improve users' lives. -- 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]
