-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(optimizer): Implement LIKE expression rule for query optimization #96
base: main
Are you sure you want to change the base?
Conversation
The commit introduces a new rule for the optimization of LIKE operator in SQL queries. The LIKE operator expressions are rewritten to make use of binary operators such as GtEq and Lt in certain cases which enhances the performance of queries. Additionally, new tests for incremented character rule have been added, and `LikeRewrite` has been added to optimizer rules in the rule set.
src/optimizer/rule/simplification.rs
Outdated
{ | ||
// if left is column and right is constant | ||
if let ScalarExpression::ColumnRef(_) = left_expr.as_ref() { | ||
if let ScalarExpression::Constant(value) = right_expr.as_ref() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduce unnecessary nesting and matching
if let ScalarExpression::Constant(DataValue::Utf8(mut val)) = right_expr.as_ref() {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
knock knock
It looks good, but it lacks the test of the rules. Reference: https://github.com/KipData/KipSQL/blob/main/src/optimizer/rule/column_pruning.rs#L166 |
} | ||
|
||
fn apply(&self, node_id: HepNodeId, graph: &mut HepGraph) -> Result<(), OptimizerError> { | ||
if let Operator::Filter(mut filter_op) = graph.operator(node_id).clone() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the operator_mut
method to modify directly instead of replace
ty, | ||
} = filter_op.predicate.clone() | ||
{ | ||
if let ScalarExpression::Constant(value) = right_expr.as_ref() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if let ScalarExpression::Constant(DataValue::Utf8(Some(value))) = right_expr.as_mut()
Instead of pattern matching layer by layer again
ty: LogicalType, | ||
filter_op: &mut FilterOperator, | ||
) { | ||
value_str.map(|value_str| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method encapsulation of little significance, just to collapse the nesting
) { | ||
value_str.map(|value_str| { | ||
if value_str.ends_with('%') { | ||
let left_bound = value_str.trim_end_matches('%'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if let Some(right_bound) = increment_last_char(left_bound) {
filter_op.predicate = Self::create_new_expr(
&mut left_expr.clone(),
ty,
left_bound.to_string(),
right_bound,
);
}
left_bound: String, | ||
right_bound: String, | ||
) -> ScalarExpression { | ||
let new_expr = ScalarExpression::Binary { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaningless variable declaration -> new_expr
@@ -127,6 +231,13 @@ mod test { | |||
use std::collections::Bound; | |||
use std::sync::Arc; | |||
|
|||
#[test] | |||
fn test_increment_char() { | |||
assert_eq!(increment_last_char("abc"), Some("abd".to_string())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are too few test cases and there is no test carry situation
The commit introduces a new rule for the optimization of LIKE operator in SQL queries. The LIKE operator expressions are rewritten to make use of binary operators such as GtEq and Lt in certain cases which enhances the performance of queries. Additionally, new tests for incremented character rule have been added, and
LikeRewrite
has been added to optimizer rules in the rule set.What problem does this PR solve?
Add corresponding issue link with summary if exists -->
Issue link:
What is changed and how it works?
Code changes
Check List
Tests
Side effects
Note for reviewer