Allow place holders like `$1` in more types of queries. #13632

davisp · 2024-12-03T20:57:22Z

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.

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 correctly.

This is a straight copy of the `datafusion/expr` directory in the `apache/datafusion` git repository. This repo exist while my PR to Datafusion isn't on crates.io. apache/datafusion#13632

alamb

Thank you @davisp -- very much appreciated.

I have some suggestions on how to make this PR better (specifically by not adding a new check, but using an existing check). However, I think we could improve it as a follow on PR as well and this could be merged as is.

Thank you again

alamb · 2024-12-04T00:11:44Z

datafusion/core/tests/dataframe/mod.rs

@@ -2014,7 +2014,7 @@ async fn test_dataframe_placeholder_missing_param_values() -> Result<()> {
    assert!(optimized_plan
        .unwrap_err()
        .to_string()
-        .contains("Placeholder type could not be resolved. Make sure that the placeholder is bound to a concrete type, e.g. by providing parameter values."));
+        .contains("Placeholder type for '$0' could not be resolved. Make sure that the placeholder is bound to a concrete type, e.g. by providing parameter values."));


alamb · 2024-12-04T00:11:57Z

datafusion/core/tests/dataframe/mod.rs

+    );
+
+    // The placeholder is not replaced with a value,
+    // so the filter data type is not know, i.e. a = $0.


Suggested change

// so the filter data type is not know, i.e. a = $0.

// so the filter data type is not known, i.e. a = $0.

Addressed here: e1d66ac

alamb · 2024-12-04T00:13:36Z

datafusion/core/tests/dataframe/mod.rs

+    // so the filter data type is not know, i.e. a = $0.
+    // Therefore, the optimization fails.
+    let optimized_plan = ctx.state().optimize(logical_plan);
+    assert!(optimized_plan.is_err());


I think this line is redundant -- as unwrap_err() will panic if optimize_plan is not an err

Addressed here: e1d66ac

alamb · 2024-12-04T00:14:11Z

datafusion/core/tests/dataframe/mod.rs

+    );
+
+    // The placeholder is not replaced with a value,
+    // so the filter data type is not know, i.e. a = $0.


Suggested change

// so the filter data type is not know, i.e. a = $0.

// so the filter data type is not known, i.e. a = $0.

Addressed here: 3d645e0

alamb · 2024-12-04T00:20:30Z

datafusion/optimizer/src/optimizer.rs

@@ -357,6 +357,7 @@ impl Optimizer {
    {
        let start_time = Instant::now();
        let options = config.options();
+        plan.validate_parameter_types()?;


Checking parameter types doesn't seem to logically belong to the optimizer which is supposed to preserve the semantics of each plan but potentially make them faster.

https://docs.rs/datafusion/latest/datafusion/optimizer/trait.OptimizerRule.html

OptimizerRules transforms one LogicalPlan into another which computes the same results, but in a potentially more efficient way. If there are no suitable transformations for the input plan, the optimizer should simply return it unmodified.

I think the Analyzer might be a more appropriate place to put this check:

https://docs.rs/datafusion/latest/datafusion/optimizer/struct.Analyzer.html#method.execute_and_check

Although thinking about it some more, if we are going to defer the error anyways, I think the cleanest solution would be to move the error to when it is needed

When I removed the specific check in the Optimizer and tried to run the queries, it failed with this error:

This feature is not implemented: Physical plan does not support logical expression Placeholder(Placeholder { id: "$1", data_type: None })

Maybe rather than a special check we could simply make this error better

(we can do this in a follow on PR)

I thought your suggestion was a lot cleaner and have pushed e5325c3 to address it. I've updated the dataframe tests, added a few to the core/tests/sql/select.rs module, and added a PREPARE test to the sqllogic test showing how things fail there as well.

Let me know if you can think of anywhere else I should add tests.

alamb · 2024-12-04T00:21:03Z

datafusion/sql/tests/sql_integration.rs

@@ -571,10 +571,6 @@ Dml: op=[Insert Into] table=[test_decimal]
    "INSERT INTO person (id, first_name, last_name) VALUES ($1, $2, $3, $4)",
    "Error during planning: Placeholder $4 refers to a non existent column"
 )]
-#[case::placeholder_type_unresolved(


What happens now in this case? Does it pass successfully?

Right, this PR is intentionally about allowing the creation of LogicalPlan's with placeholders. Previously, LogicalPlan creation would fail when any placeholder is part of the schema. Given that all this test is doing is creating LogicalPlan's, this example now passes. I thought about moving it to a new test that explicitly shows it failing at the optimization step but then felt that that case was already covered by the other test updates.

datafusion/expr/src/expr_schema.rs

alamb · 2024-12-04T00:33:30Z

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 great ❤️

Previously, these errors would occurr during optimization. Now that we allow unbound placeholders through the optimizer they fail when creating the physical plan instead.

alamb

This is beautiful -- thank you so much @davisp

* Allow place holders in the column list 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 correctly. * Remove redundant asserts * Fix typo in comments * Add comment explaining DataType::Null * Move unbound placeholder error to physical-expr Previously, these errors would occurr during optimization. Now that we allow unbound placeholders through the optimizer they fail when creating the physical plan instead. * Fix expected error message

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Dec 3, 2024

davisp mentioned this pull request Dec 3, 2024

Optimizing LogicalPlan with placeholders fails #8819

Closed

alamb approved these changes Dec 4, 2024

View reviewed changes

davisp added 3 commits December 4, 2024 09:13

Remove redundant asserts

e1d66ac

Fix typo in comments

3d645e0

Add comment explaining DataType::Null

4be985a

github-actions bot added physical-expr Physical Expressions and removed optimizer Optimizer rules labels Dec 4, 2024

Move unbound placeholder error to physical-expr

e5325c3

Previously, these errors would occurr during optimization. Now that we allow unbound placeholders through the optimizer they fail when creating the physical plan instead.

davisp force-pushed the allow-untyped-prepare-parameters branch from 53f8e91 to e5325c3 Compare December 4, 2024 17:13

Fix expected error message

ea70383

alamb approved these changes Dec 5, 2024

View reviewed changes

alamb changed the title ~~Allow place holders in the column list~~ Allow place holders like $1 in more types of queries. Dec 5, 2024

alamb merged commit b30c200 into apache:main Dec 5, 2024
26 checks passed

jonahgao mentioned this pull request Dec 5, 2024

[Epic] Prepared Statement Support #4539

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow place holders like `$1` in more types of queries. #13632

Allow place holders like `$1` in more types of queries. #13632

davisp commented Dec 3, 2024

alamb left a comment

alamb Dec 4, 2024

alamb Dec 4, 2024

davisp Dec 4, 2024

alamb Dec 4, 2024

davisp Dec 4, 2024

alamb Dec 4, 2024

davisp Dec 4, 2024

alamb Dec 4, 2024

alamb Dec 4, 2024

alamb Dec 4, 2024

davisp Dec 4, 2024

alamb Dec 4, 2024

davisp Dec 4, 2024

alamb commented Dec 4, 2024

alamb left a comment •

edited

Loading

	// so the filter data type is not know, i.e. a = $0.
	// so the filter data type is not known, i.e. a = $0.

Allow place holders like $1 in more types of queries. #13632

Allow place holders like $1 in more types of queries. #13632

Conversation

davisp commented Dec 3, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 4, 2024

alamb left a comment • edited Loading

Choose a reason for hiding this comment

Allow place holders like `$1` in more types of queries. #13632

Allow place holders like `$1` in more types of queries. #13632

alamb left a comment •

edited

Loading