myandpr commented on code in PR #20965:
URL: https://github.com/apache/datafusion/pull/20965#discussion_r2944203969
##########
datafusion/sql/src/planner.rs:
##########
@@ -615,6 +617,78 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
})
}
+ pub(crate) fn validate_expr_type(
+ &self,
+ expr: &Expr,
+ schema: &DFSchema,
+ ) -> Result<()> {
+ expr.apply(|expr| {
+ self.validate_expr_node_type(expr, schema)?;
+ Ok(TreeNodeRecursion::Continue)
+ })?;
+ Ok(())
+ }
+
+ pub(crate) fn validate_filter_predicate(
+ &self,
+ expr: &Expr,
+ schema: &DFSchema,
+ ) -> Result<()> {
+ self.validate_expr_type(expr, schema)?;
+ let predicate_type = expr.get_type(schema)?;
+ if !Self::is_allowed_predicate_type(&predicate_type) {
+ return plan_err!(
+ "Cannot create filter with non-boolean predicate '{expr}'
returning {predicate_type}"
Review Comment:
Thanks @alamb , that makes sense.
I agree that doing this in the SQL planner makes the behavior SQL-specific
and misses queries built through the DataFrame API.
I’ll rework this toward the analyzer/type coercion path instead, and I’ll
take a closer look at which cases are already covered there versus which ones
still need to be handled. I’ll also update the regression tests to match that
direction.
--
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]
