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.

Sign up for GitHub

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

Jump to bottom

feat(batch): enable parallel table scan #3670

Merged
merged 15 commits into from
Jul 6, 2022
Merged

feat(batch): enable parallel table scan #3670

merged 15 commits into from
Jul 6, 2022

Conversation

xxchan
Copy link
Member

@xxchan xxchan commented Jul 5, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

We will not maintain the order of select from an MV with ORDER BY clause now. #3668 (comment)

Documentation

If your pull request contains user-facing changes, please specify the types of the changes, and create a release note. Otherwise, please feel free to remove this section.

Types of user-facing changes

Please keep the types that apply to your changes, and remove those that do not apply.

  • SQL commands, functions, and operators

Release note

Select from an MV with ORDER BY clause may return unordered results.

Refer to a related PR or issue link (optional)

close #3583
close #3237

@codecov
Copy link

codecov bot commented Jul 5, 2022 •
edited
Loading

Codecov Report

Merging #3670 (ecf0e8d) into main (d0739a1) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3670      +/-   ##
==========================================
+ Coverage   74.30%   74.41%   +0.11%     
==========================================
  Files         791      791              
  Lines      111592   111589       -3     
==========================================
+ Hits        82913    83038     +125     
+ Misses      28679    28551     -128
Flag Coverage Δ
rust 74.41% <100.00%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
...ontend/src/optimizer/plan_node/batch_simple_agg.rs 84.00% <100.00%> (+0.21%) ⬆️
src/frontend/src/scheduler/plan_fragmenter.rs 92.76% <100.00%> (+4.06%) ⬆️
src/storage/src/table/storage_table.rs 85.83% <100.00%> (ø)
src/meta/src/manager/id.rs 95.50% <0.00%> (-0.57%) ⬇️
src/frontend/src/expr/utils.rs 98.99% <0.00%> (-0.26%) ⬇️
src/storage/src/hummock/local_version_manager.rs 81.49% <0.00%> (+0.11%) ⬆️
src/meta/src/hummock/mock_hummock_meta_client.rs 41.50% <0.00%> (+0.94%) ⬆️
src/frontend/src/scheduler/distributed/query.rs 74.07% <0.00%> (+4.84%) ⬆️
src/frontend/src/scheduler/worker_node_manager.rs 74.19% <0.00%> (+11.82%) ⬆️
... and 2 more

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

BugenZhao
BugenZhao reviewed Jul 6, 2022
View reviewed changes
Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

The changes LGTM if we agree on not preserving order on mv scans. 🤣

src/frontend/src/scheduler/plan_fragmenter.rs Outdated Show resolved Hide resolved
@BugenZhao
use unordered iter
fb089b0 
Signed-off-by: Bugen Zhao <[email protected]>
liurenjie1024
liurenjie1024 reviewed Jul 6, 2022
View reviewed changes
Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Good job! Generally LGTM, requires small refine.

e2e_test/batch/basic/array.slt.part Outdated
@@ -1,3 +1,5 @@
control sortmode rowsort
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this? This is not querying from mv.

Copy link
Member Author

Choose a reason for hiding this comment

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

How's querying from table different from querying from MV? 🤔 Also partitioned and results are unordered?

Copy link
Contributor

Choose a reason for hiding this comment

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

They are same. But as we have discussed, we should not use rowsort as default behavior, which is error prone.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK so I will move rowsort to queries

e2e_test/batch/basic/boolean.slt.part Outdated
@@ -1,3 +1,5 @@
control sortmode rowsort
Copy link
Contributor

Choose a reason for hiding this comment

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

Also remove this?

src/frontend/src/scheduler/plan_fragmenter.rs Outdated
// Tracking issue: <https:/singularity-data/risingwave/issues/3583>
let is_singleton = true;
// let is_singleton = builder.parallelism == 1;
let is_singleton = builder.parallelism == 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about remove is_singleton? Since we no longer need it.

@xxchan xxchan added the user-facing-changes Contains changes that are visible to users label Jul 6, 2022
liurenjie1024
liurenjie1024 approved these changes Jul 6, 2022
View reviewed changes
Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

LSTM!!

@mergify mergify bot merged commit f1f3b4b into main Jul 6, 2022
@mergify mergify bot deleted the xxchan/parallel branch July 6, 2022 10:18
@hengm3467
Copy link
Contributor

@xxchan Could you please explain the release note part ? It sounds like a limitation. Does this mean that this PR resolves the issue (order by statements return unordered results)? Thanks.

@xxchan
Copy link
Member Author

xxchan commented Aug 9, 2022

@xxchan Could you please explain the release note part ? It sounds like a limitation. Does this mean that this PR resolves the issue (order by statements return unordered results)? Thanks.

The problem is described here https://singularity-data.slack.com/archives/C036F5Z3EMD/p1657075038333919

nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022
@xxchan @BugenZhao
@mergify @nasnoisaac
feat(batch): enable parallel table scan (risingwavelabs#3670)
b092479 
* feat(batch): enable parallel table scan

* Single distinct agg

* use unordered iter

Signed-off-by: Bugen Zhao <[email protected]>

* add sortmode

* fix distinct agg & remove is_singleton

* move rowsort to queries

* clippy

* update comment

* add more rowsort

* last rowsort

* final rowsort

* fix non deterministic order by

Co-authored-by: Bugen Zhao <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BugenZhao BugenZhao BugenZhao left review comments

@liurenjie1024 liurenjie1024 liurenjie1024 approved these changes

@fuyufjh fuyufjh Awaiting requested review from fuyufjh

@st1page st1page Awaiting requested review from st1page

Assignees
No one assigned
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test: order of e2e test results batch: True parallel table scan.
4 participants
@xxchan @hengm3467 @liurenjie1024 @BugenZhao