kczimm commented on code in PR #15980:
URL: https://github.com/apache/datafusion/pull/15980#discussion_r2080118522
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1494,6 +1494,14 @@ impl LogicalPlan {
let mut param_types: HashMap<String, Option<DataType>> =
HashMap::new();
self.apply_with_subqueries(|plan| {
+ if let LogicalPlan::Limit(Limit { fetch: Some(e), .. }) = plan {
Review Comment:
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. For example, if I modify the test and call
`LogicalPlan::with_param_values`, it will attempt to infer the datatypes so
that replacement is valid. I've done this and the type is still not getting
inferred for the `LIMIT` case. This makes sense though the way it's currently
designed and the nature of the LIMIT clause. `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,
https://github.com/apache/datafusion/blob/41e7aed3a943134c40d1b18cb9d424b358b5e5b1/datafusion/opt
imizer/src/analyzer/type_coercion.rs#L242.
--
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]
