Additional placeholder datatype inferencing #15980

kczimm · 2025-05-07T15:04:53Z

Which issue does this PR close?

Rationale for this change

Increases functionality of parameterized queries.

What changes are included in this PR?

Placeholder datatype inference after LIMIT clause and Expr::InSubquery is now supported.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, users can now use parameterized queries in more places.

only infer subquery if exactly 1 field

alamb

Thanks @kczimm -- this looks great to me except for the special case for limit -- something seems to be wrong there. Would you be willing to help debug it further?

alamb · 2025-05-07T20:11:54Z

datafusion/expr/src/expr.rs

+                    let fields = subquery_schema.fields();
+
+                    // only supports subquery with exactly 1 field
+                    if let [first_field] = &fields[..] {


What happens if there is more than one field? Will it not rewrite any placeholders?

The subquery should only return one column. https://github.com/apache/datafusion/blob/main/datafusion/sql/src/expr/subquery.rs#L120

alamb · 2025-05-07T20:15:25Z

datafusion/expr/src/logical_plan/plan.rs

                                param_types.insert(id.clone(), Some(dt.clone()));
                            }
+                            (Some(Some(_)), None) => {
+                                // we have already inferred the datatype


When can this happen? Like what situations would one instance of the parameter like $2 be resolved but another instance would not be 🤔

This happens because we earlier treated LIMIT specially. We added it to the param_types but when we get to the Placeholder expression here, we see that the type was never inferred. If we weren't in an immutable method, we could do the actual type inference above instead of directly populating param_types and let it be appropriately populated here.

alamb · 2025-05-07T20:17:59Z

datafusion/expr/src/logical_plan/plan.rs

        let mut param_types: HashMap<String, Option<DataType>> = HashMap::new();

        self.apply_with_subqueries(|plan| {
+            if let LogicalPlan::Limit(Limit { fetch: Some(e), .. }) = plan {


I don't understand why the expression in the LIMIT needs special handling. It seems like it should be handled by LogicalPlan::apply_expressions here https://github.com/apache/datafusion/blob/41e7aed3a943134c40d1b18cb9d424b358b5e5b1/datafusion/expr/src/logical_plan/tree_node.rs#L462-L461

Also this code doesn't seem to handle the offset clause, only the fetch

However, when I removed this special case the tests you have added definitely fail 🤔 Something really strange is going on

My test is actually really unfair, I will modify it. I constructed LogicalPlan by hand and gave it no opportunity to do any placeholder type inference.

Expr::infer_placeholder_types takes an Expr and a DFSchema and it uses the schema to infer the datatype from the field type. However, there is no schema that will provide that information for LIMIT. It's a special case and we (I believe) always know what datatype it must be and it need not be inferred. I think the case is the same for offset as you mention. In fact, it looks like we try to coerce it to be Int64,

datafusion/datafusion/optimizer/src/analyzer/type_coercion.rs

Line 242 in 41e7aed

/// Coerce the fetch and skip expression to Int64 type.

.

What concerns me about the code is that code for inferring data type has a special case for LIMIT but from what I know LIMIT doesn't have any special behavior for parameters

If we need a schema to infer types for LIMIT we could either

Use its input plan (though all schema would be ignored)

Use Schema::empty() perhaps

Does that make sense?

This is my last remaining concern -- if we can figure out some way that LIMIT doesn't need special case it would be great

Yeah I agree with you. I've got caught up working on some other things but I've been giving this some thought and plan to work on it. We will find something that we like.

Thank you 🙏

UPSTREAM NOTE: Upstream PR has been created but not merged yet. Should be available in DF49 apache#15980