jayzhan211 commented on code in PR #25364:
URL: https://github.com/apache/datafusion/pull/25364#discussion_r4173315819
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -355,6 +355,14 @@ impl<'a> TypeCoercionRewriter<'a> {
schema: &DFSchema,
expr_name: &str,
) -> Result<Expr> {
+ // An untyped placeholder has no type to coerce yet. Leave it bare
so
+ // `LogicalPlan::get_parameter_fields` can still default it to
Int64
+ // post-analysis; wrapping it in a `CAST` here would hide it from
that
+ // inference because the cast's inner expression is no longer a
+ // top-level `Limit` operand.
+ if matches!(&expr, Expr::Placeholder(p) if p.field.is_none()) {
Review Comment:
Leaving the placeholder uncast changes what the analyzer emits. Analyze →
bind `Int32(1)` → optimize now leaves `fetch=Int32(1)`, which `get_fetch_type`
rejects (`Unsupported LIMIT expression: Int32(1)`). On main the cast folds to
`Int64(1)`. A client that binds the reported type hits this with `SELECT $1 +
CAST(1 AS INT) AS v LIMIT $1`, where `$1` is reported as `Int32`. Keeping the
cast and looking through it in `get_parameter_fields` avoids that; every test
in this PR still passes.
```rs
let analyzed = Analyzer::new().execute_and_check(plan, &config, |_, _| {})?;
let bound = analyzed.with_param_values(vec![ScalarValue::Int32(Some(1))])?;
let optimized = Optimizer::new().optimize(bound, &OptimizerContext::new(),
|_, _| {})?;
// main: fetch=Int64(1); this PR: fetch=Int32(1) ->
FetchType::UnsupportedExpr
```
Drop the early return (and its comment):
```diff
- if matches!(&expr, Expr::Placeholder(p) if p.field.is_none()) {
- return Ok(expr);
- }
```
`datafusion/expr/src/logical_plan/plan.rs` (also add `Cast` to the
`crate::expr` import):
```diff
- if matches!(plan, LogicalPlan::Limit(_))
- && let Expr::Placeholder(Placeholder { id, field: None
}) = expr
- {
- row_count_parameters.insert(id.clone());
+ if let LogicalPlan::Limit(_) = plan {
+ // `TypeCoercion` wraps LIMIT/OFFSET operands in
`CAST(.. AS Int64)`
+ let operand = match expr {
+ Expr::Cast(Cast { expr, field })
+ if field.data_type() == &DataType::Int64 =>
+ {
+ expr.as_ref()
+ }
+ expr => expr,
+ };
+ if let Expr::Placeholder(Placeholder { id, field: None
}) = operand {
+ row_count_parameters.insert(id.clone());
+ }
}
```
--
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]
