-
Notifications
You must be signed in to change notification settings - Fork 214
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): support column pruning for some operators #653
Conversation
That's so cool! I'll take a look later. cc @st1page may also help 🥰 |
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.
I think it generally LGTM from the high level. But there are too many details in such a big PR which is hard to review... 🥹
let child_schema_len = self.child.out_types().len(); | ||
let mut input_cols = BitSet::with_capacity(child_schema_len); | ||
(0..child_schema_len).into_iter().for_each(|col_idx| { | ||
input_cols.insert(col_idx); | ||
}); |
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 collect
like other nodes?
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.
I will modify it
@@ -8,10 +8,23 @@ use super::*; | |||
|
|||
/// A dummy plan. | |||
#[derive(Debug, Clone, Serialize)] | |||
pub struct Dummy {} | |||
pub struct Dummy { | |||
schema: Vec<ColumnDesc>, |
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.
Why Dummy
needs schema?
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.
Maybe it was just a bug and I fixed it by the way
In SQL: SELECT x FROM test WHERE 1 > 3
.
the projection operator schema()
requires the schema of the following operator, schema()
will panic if dummy schema hasn't schema.
let input_cols = visitor.0; | ||
|
||
if new_projection_expressions.is_empty() { | ||
new_projection_expressions.push(BoundExpr::Constant(DataValue::Int32(81))); |
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.
What's this?
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.
81 is a magic number.
If we have a existential subquery SELECT v1 FROM v_table WHERE EXISTS (SELECT * FROM w_table)
, we don't need any column of w_table in the projection(expr: v1). After column pruning, the subquery projection expression len will become 0. But maybe we should a constant expression for upper operator in some subquery implementation. But for now , the 81 is not important.
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 there's no output column, maybe we'd better refactor executor/chunk to allow such situation. Adding it here isn't correct in semantics: the required_cols
isn't satisfied.
let group_keys_len = self.group_keys.len(); | ||
|
||
// Collect ref_idx of AggCall args | ||
let mut visitor = CollectRequiredCols(BitSet::new()); |
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.
It will be safer to create a BitSet
with explicit capacity. Not sure whether it will cause problem here.
Signed-off-by: lokax <[email protected]>
Signed-off-by: lokax <[email protected]>
Signed-off-by: lokax <[email protected]>
Signed-off-by: lokax <[email protected]>
Signed-off-by: lokax <[email protected]>
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.
Others generally LGTM. Let's merge it!
Signed-off-by: xxchan <[email protected]>
Signed-off-by: xxchan <[email protected]>
Signed-off-by: lokax [email protected]
#654