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(planner): Support select operator in new planner framework #5059

Merged
merged 3 commits into from
Apr 26, 2022

Conversation

leiysky
Copy link
Contributor

@leiysky leiysky commented Apr 26, 2022

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Support WHERE clause in new planner.

Should be merged after #5058

Changelog

  • New Feature

Related Issues

Related to #3747

@vercel
Copy link

vercel bot commented Apr 26, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
databend ✅ Ready (Inspect) Visit Preview Apr 26, 2022 at 11:45AM (UTC)

@mergify
Copy link
Contributor

mergify bot commented Apr 26, 2022

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@mergify mergify bot added the pr-feature this PR introduces a new feature to the codebase label Apr 26, 2022
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

👍,it works for some cases

mysql> select * from t;
+------+
| a    |
+------+
|    1 |
|    2 |
+------+
2 rows in set (0.07 sec)
Read 2 rows, 8 B in 0.019 sec., 103.73 rows/sec., 414.93 B/sec.

mysql> select a from t where a = 1;
ERROR 1105 (HY000): Code: 1002, displayText = Unsupported expr: Literal(Number("1")).
mysql> select a from t where a = a;
+------+
| a    |
+------+
|    1 |
|    2 |
+------+
2 rows in set (0.04 sec)
Read 2 rows, 8 B in 0.006 sec., 346.2 rows/sec., 1.38 KB/sec.

) -> Result<ScalarExprRef> {
let left_scalar = self.bind_expr(left_child, bind_context)?;
let right_scalar = self.bind_expr(right_child, bind_context)?;
match op {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to match the op here, can't we just leave it to Expression?

--- a/query/src/sql/planner/binder/scalar.rs
+++ b/query/src/sql/planner/binder/scalar.rs
@@ -64,16 +64,10 @@ impl ScalarBinder {
     ) -> Result<ScalarExprRef> {
         let left_scalar = self.bind_expr(left_child, bind_context)?;
         let right_scalar = self.bind_expr(right_child, bind_context)?;
-        match op {
-            BinaryOperator::Eq => Ok(Arc::new(Scalar::Equal {
-                left: left_scalar,
-                right: right_scalar,
-            })),
-            _ => Err(ErrorCode::UnImplement(format!(
-                "Unsupported binary operator: {}",
-                op.to_string()
-            ))),
-        }
+        Ok(Arc::new(Scalar::Binary {
+            left: left_scalar,
+            right: right_scalar,
+        }))
     }
 }

Then other ops will be supported

mysql> set enable_planner_v2=1;
No connection. Trying to reconnect...
Connection id:    8
Current database: *** NONE ***

Query OK, 0 rows affected (0.56 sec)
Read 0 rows, 0 B in 0.537 sec., 0 rows/sec., 0 B/sec.

mysql> select * from t;
+------+
| a    |
+------+
|    1 |
|    2 |
+------+
2 rows in set (0.06 sec)
Read 2 rows, 8 B in 0.017 sec., 115.6 rows/sec., 462.41 B/sec.

mysql> select a from t where a <= a;
+------+
| a    |
+------+
|    1 |
|    2 |
+------+
2 rows in set (0.04 sec)
Read 2 rows, 8 B in 0.005 sec., 378.62 rows/sec., 1.51 KB/sec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some binary operators have useful property, I'd like to make them typed.

More scalar expressions would be supported later.

Copy link
Member

Choose a reason for hiding this comment

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

The useful property will be used in the optimizer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for instance cardinality estimation.

Signed-off-by: leiysky <leiysky@outlook.com>
Signed-off-by: leiysky <leiysky@outlook.com>
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify
Copy link
Contributor

mergify bot commented Apr 26, 2022

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-review pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants