kosiew commented on code in PR #21667:
URL: https://github.com/apache/datafusion/pull/21667#discussion_r3135556737
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -1365,9 +1346,12 @@ fn rewrite_projection(
}
}
-/// Creates a new LogicalPlan::Filter node.
-pub fn make_filter(predicate: Expr, input: Arc<LogicalPlan>) ->
Result<LogicalPlan> {
- Filter::try_new(predicate, input).map(LogicalPlan::Filter)
+/// Creates a filter node without re-validating predicate type.
+fn make_filter(predicate: Expr, input: Arc<LogicalPlan>) ->
Result<LogicalPlan> {
Review Comment:
`make_filter()` no longer seems to have a failure path, but it still returns
`Result<LogicalPlan>`. That makes the call sites read as though validation or
another fallible operation is still happening here.
Would it be clearer for this helper to return `LogicalPlan`, or maybe
`Filter`, directly and keep `Result` only around the rewrite steps that can
actually fail?
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2496,6 +2496,19 @@ impl Filter {
Self::try_new_internal(predicate, input)
}
+ /// Create a new filter operator without re-validating the predicate type.
+ ///
+ /// This is intended for internal optimizer use when rearranging predicates
+ /// that are already known to be valid filter expressions. Like
+ /// [`Self::try_new`], this removes nested aliases from the predicate.
+ #[doc(hidden)]
+ pub fn new_unchecked(predicate: Expr, input: Arc<LogicalPlan>) -> Self {
Review Comment:
I am a bit concerned about making `new_unchecked` public here.
`#[doc(hidden)]` keeps it out of the generated docs, but it is still part of
the public API and can be called by external users.
Even though `Filter` has public fields today, adding a named public
unchecked constructor makes it much easier and more discoverable to bypass the
boolean-type validation. Could this stay `pub(crate)`, or use another
crate-private visibility, so the unchecked path remains scoped to optimizer
internals?
--
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]
