Make expression manipulation consistent and easier to use: combine/split filter conjunction, etc
#3810
Conversation
github-actions
bot
added
core
| actual += "\n"; | ||
| } | ||
| actual += &format!("{}", plan.display_indent()); | ||
| use std::fmt::Write as _; |
There was a problem hiding this comment.
clippy told me to do this
| LogicalPlan::Filter(Filter { input, predicate }) => { | ||
| let mut predicates = vec![]; | ||
| utils::split_conjunction(predicate, &mut predicates); | ||
| let predicates = utils::split_conjunction(predicate); |
There was a problem hiding this comment.
here is an example where the split_conjuntion API is easier to use now
| let subqry_alias = format!("__sq_{}", optimizer_config.next_id()); | ||
| let mut subqry_plan = LogicalPlanBuilder::from((*subqry_input).clone()); | ||
| if let Some(expr) = combine_filters(&other_subqry_exprs) { | ||
| if let Some(expr) = conjunction(other_subqry_exprs) { |
There was a problem hiding this comment.
here is an example of less copying (can use other_subqry_exprsdirectly)
alamb
force-pushed
the
alamb/better_expr_api
branch
2 times, most recently
from
ae47468 to
6e45c74
Compare
| /// Splits an owned conjunctive [`Expr`] such as `A AND B AND C` => `[A, B, C]` | ||
| /// | ||
| /// See [`split_conjunction`] for more details. | ||
| pub fn split_conjunction_owned(expr: Expr) -> Vec<Expr> { |
There was a problem hiding this comment.
this function used to be called uncombine_filters which was not at all consistent with the split_conjunction that took a reference 🤯
…e conjunction/disjunction and reduce clone
alamb
force-pushed
the
alamb/better_expr_api
branch
from
6e45c74 to
0dd3db4
Compare
alamb
changed the title
combine/split filter conjunction, etccombine/split filter conjunction, etc
| } | ||
| Expr::Alias(expr, _) => { | ||
| split_conjunction(expr, predicates); | ||
| Expr::Alias(expr, _) => split_conjunction_impl(expr, exprs), |
| fn combine_zero_filters() { | ||
| let result = combine_filters(&[]); | ||
| assert_eq!(result, None); | ||
| fn test_split_conjunction() { |
There was a problem hiding this comment.
IMO, I think we can add test for conjunction(). At the same time, we can check the tree structure of this expression by using match.
expr is (A B) C;
/// using `match` to check.
match expr {
And(
And(
B,
C
),
C
)
}[A, B, C ,D , E] -> (((A B) C) (D E)) is different from ((((A B) C) D) E).
we can see the result of conjunction() in the UT, ensure the result of tree structure of conjunction()
There was a problem hiding this comment.
This was a great idea @jackwener -- thank you. I added this test 09f3cf4
As part of writing tests, I also found that the API for disjunction was slightly different than conjunction so I made them the same as well.
|
Benchmark runs are scheduled for baseline = 0b90a8a and contender = fc5081d. fc5081d is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
3 participants
DRAFT as it builds on #3809Which issue does this PR close?
Closes #3808
Rationale for this change
The APIs for manipulating expressions were all over the map (sometimes return Vec, sometimes taking them as mut, owned, etc) as well as being inconsistently named and inconsistently tested.
In fact I couldn't find
uncombine_filter(which is similar, but not the same assplit_conjunction)What changes are included in this PR?
Change the APIs to be consistently named and take reasonable arguments
split_conjunctionto return aVecthus simplifying the APIsplit_conjunction_ownedto make it clear how it is related tosplit_conjunctioncombine_filtertoconjunctioncombine_filter_disjunctiontodisjunctionconjunctionanddisjunctionto avoid clonesI will highlight the changes inline.
Are there any user-facing changes?
If anyone uses these APIs they will need to change them, but hopfully