fix: infer LIMIT and OFFSET placeholder parameter types#23260
fix: infer LIMIT and OFFSET placeholder parameter types#23260Probablism wants to merge 1 commit into
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
There was a problem hiding this comment.
@Probablism
Thanks for the fix.
I found one issue that looks like it can make the new parameter metadata report the wrong type when a LIMIT or OFFSET placeholder is reused in an unconstrained expression.
| (_, Some(field)) => { | ||
| param_types.insert(id.clone(), Some(Arc::clone(field))); | ||
| } | ||
| _ => { |
There was a problem hiding this comment.
collect_parameter_fields can currently erase the Int64 type inferred from a LIMIT or OFFSET placeholder when the same parameter is later seen in an untyped context.
For example, SELECT $1 FROM person LIMIT $1 records $1 as Int64 from the row-count expression first because apply_with_subqueries visits the Limit node before its child plans. Later, the projection's bare $1 reaches this catch-all arm and overwrites that entry with None. As a result, the metadata API still reports the row-count parameter as untyped, which breaks the invariant this PR is trying to enforce when the placeholder is reused in an unconstrained expression.
Could we keep an existing Some(field) when the new occurrence is None, and only insert None if the parameter is not already known? Please also add a regression test for a LIMIT or OFFSET placeholder reused in an otherwise-untyped expression.
| let mut param_types: HashMap<String, Option<FieldRef>> = HashMap::new(); | ||
|
|
||
| self.apply_with_subqueries(|plan| { | ||
| let row_count_field = Arc::new(Field::new("", DataType::Int64, true)); |
There was a problem hiding this comment.
Small polish suggestion: consider hoisting row_count_field outside the apply_with_subqueries closure so the Int64 field is allocated once per get_parameter_fields call instead of once per visited plan node.
This is not blocking, but I think it would also make the row-count placeholder invariant a little easier to spot while reading the code.
Which issue does this PR close?
LIMITclause #15978.Rationale for this change
Bare placeholders used as
LIMITorOFFSETrow-count expressions currently remain untyped inLogicalPlan::get_parameter_types(). DataFusion already coerces LIMIT/OFFSET row counts toInt64during analysis, so the metadata APIs should report the same type for bare row-count placeholders.What changes are included in this PR?
Int64parameter fields for bare unresolved placeholders used directly asLIMITorOFFSETexpressions.Are these changes tested?
Yes:
cargo fmt --all -- --checkcargo test -p datafusion-expr test_limit_cargo test -p datafusion-sql --test sql_integration test_infer_types_from_limitcargo test -p datafusion-sql --test sql_integration test_prepare_statement_infer_types_from_limitcargo test -p datafusioncargo test -p datafusion-clicargo test --profile=ci --test sqllogictests./dev/rust_lint.shAre there any user-facing changes?
Yes.
LogicalPlan::get_parameter_types()andLogicalPlan::get_parameter_fields()now reportInt64for bare placeholders used directly inLIMITandOFFSET. There are no breaking API changes.