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

test(optimizer): ensure column prune correctly handle reorder #2603

Merged
merged 2 commits into from
May 17, 2022

Conversation

Enter-tainer
Copy link
Contributor

What's changed and what's your intention?

This PR implements the missing feature described in #2095. This allows us to adjust the order of columns while performing column prune. Currently, this is done by inserting an extra logical_project node and will be replaced with the output_index described in #1922 in the future.

This PR only contains the implementation for logical_agg and logical_join, implementations for other nodes would come in later PRs.

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)

related: #2095, #1922

Currently done for logical_agg and logical_join
@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #2603 (9b7dd91) into main (48e898e) will increase coverage by 0.05%.
The diff coverage is 96.61%.

❗ Current head 9b7dd91 differs from pull request most recent head c1b8b59. Consider uploading reports for the commit c1b8b59 to get more accurate results

@@            Coverage Diff             @@
##             main    #2603      +/-   ##
==========================================
+ Coverage   72.36%   72.41%   +0.05%     
==========================================
  Files         678      675       -3     
  Lines       88069    88016      -53     
==========================================
+ Hits        63730    63740      +10     
+ Misses      24339    24276      -63     
Flag Coverage Δ
rust 72.41% <96.61%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
...c/frontend/src/optimizer/plan_node/logical_join.rs 88.06% <93.65%> (+0.54%) ⬆️
...rc/frontend/src/optimizer/plan_node/logical_agg.rs 97.73% <100.00%> (+0.07%) ⬆️
src/storage/src/object/mod.rs 13.63% <0.00%> (-14.20%) ⬇️
src/storage/src/object/mem.rs 78.21% <0.00%> (-2.43%) ⬇️
src/storage/src/store_impl.rs 5.79% <0.00%> (-1.48%) ⬇️
src/storage/src/object/disk.rs 94.76% <0.00%> (-0.96%) ⬇️
src/common/src/config.rs 79.16% <0.00%> (-0.51%) ⬇️
src/storage/src/hummock/compactor.rs 64.28% <0.00%> (-0.24%) ⬇️
src/storage/src/hummock/test_utils.rs 82.85% <0.00%> (-0.17%) ⬇️
src/frontend/src/binder/expr/function.rs 91.37% <0.00%> (-0.08%) ⬇️
... and 18 more

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

@fuyufjh fuyufjh requested review from xxchan, xiangjinwu and st1page May 17, 2022 13:51
Copy link
Contributor

@xiangjinwu xiangjinwu left a comment

Choose a reason for hiding this comment

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

Lgtm, but feels like the title and description is a little bit off. This is not the implementation of reordering with Project - it is mostly done in #2095 (despite minor change for LogicalAgg). The content of this PR is mostly adding tests to assure that reordering works as expected.

@Enter-tainer Enter-tainer changed the title feat(optimizer): allow column prune with required order test(optimizer): ensure column prune correctly handle reorder May 17, 2022
@github-actions github-actions bot added component/test Test related issue. and removed type/feature labels May 17, 2022
@Enter-tainer Enter-tainer enabled auto-merge (squash) May 17, 2022 14:42
@Enter-tainer Enter-tainer merged commit bd491eb into main May 17, 2022
@Enter-tainer Enter-tainer deleted the lwz/allow-reorder-in-column-prune branch May 17, 2022 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/test Test related issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants