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): col prune for hop #2030

Merged
merged 6 commits into from
Apr 22, 2022
Merged

feat(optimizer): col prune for hop #2030

merged 6 commits into from
Apr 22, 2022

Conversation

st1page
Copy link
Contributor

@st1page st1page commented Apr 21, 2022

What's changed and what's your intention?

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #2030 (bb4269c) into main (82e538b) will increase coverage by 0.18%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##             main    #2030      +/-   ##
==========================================
+ Coverage   70.65%   70.84%   +0.18%     
==========================================
  Files         629      629              
  Lines       80987    81035      +48     
==========================================
+ Hits        57223    57408     +185     
+ Misses      23764    23627     -137     
Flag Coverage Δ
rust 70.84% <94.11%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...tend/src/optimizer/plan_node/logical_hop_window.rs 95.20% <92.68%> (+95.20%) ⬆️
...c/frontend/src/optimizer/plan_node/logical_scan.rs 97.05% <100.00%> (+0.02%) ⬆️
src/frontend/src/utils/column_index_mapping.rs 83.00% <100.00%> (+0.59%) ⬆️
src/common/src/types/ordered_float.rs 23.50% <0.00%> (-0.20%) ⬇️
src/common/src/types/mod.rs 64.07% <0.00%> (+0.19%) ⬆️
src/common/src/types/interval.rs 78.91% <0.00%> (+0.54%) ⬆️
...ntend/src/binder/relation/window_table_function.rs 70.93% <0.00%> (+2.32%) ⬆️
src/common/src/catalog/schema.rs 84.73% <0.00%> (+6.10%) ⬆️
src/frontend/src/planner/relation.rs 91.74% <0.00%> (+21.10%) ⬆️
...ntend/src/optimizer/plan_node/stream_hop_window.rs 63.15% <0.00%> (+63.15%) ⬆️
... and 1 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

}

pub fn i2o_col_mapping(&self) -> ColIndexMapping {
ColIndexMapping::identity_or_none(self.input.schema().len(), self.schema().len())
Copy link
Collaborator

Choose a reason for hiding this comment

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

identity is enough here?

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, and the added window_start and window_end have not mapping to the input columns.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So why identity_or_none instead of identity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in fact, I think the identity_or_none actually means identity_with_target_size, which means it will be identity for intersection of [0..source_size] and [0..target_size], and there will be no mapping for others

proj_map.put(new_hop.schema().len() - 2, None);
LogicalProject::with_mapping(new_hop.into(), proj_map)
}
(false, false) => unreachable!(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO it's a valid SQL that user use HOP but don't really use window_start and window_end

@st1page
Copy link
Contributor Author

st1page commented Apr 21, 2022

PTAL @TennyZhuang

@TennyZhuang TennyZhuang merged commit ae9ff0f into main Apr 22, 2022
@TennyZhuang TennyZhuang deleted the sts/hop_col_prune branch April 22, 2022 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants