Skip to content
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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

loloxwg
Copy link
Member

@loloxwg loloxwg commented Nov 12, 2023

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

  • Has Rust code change
  • Has CI related scripts change

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Note for reviewer

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.
@loloxwg loloxwg changed the title feat(optimizer): Implement LIKE operator rule for query optimization feat(optimizer): Implement LIKE expression rule for query optimization Nov 12, 2023
@loloxwg loloxwg requested a review from KKould November 12, 2023 13:12
src/db.rs Outdated Show resolved Hide resolved
{
// 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() {
Copy link
Member

@KKould KKould Nov 12, 2023

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() {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

knock knock

@KKould KKould requested a review from lewiszlw November 12, 2023 16:09
@KKould
Copy link
Member

KKould commented Dec 8, 2023

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() {
Copy link
Member

@KKould KKould Dec 25, 2023

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() {
Copy link
Member

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| {
Copy link
Member

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('%');
Copy link
Member

@KKould KKould Dec 25, 2023

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 {
Copy link
Member

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()));
Copy link
Member

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

@KKould KKould added the enhancement New feature or request label Dec 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants