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(batch): index scan & point get #3014

Merged
merged 26 commits into from
Jun 9, 2022
Merged

feat(batch): index scan & point get #3014

merged 26 commits into from
Jun 9, 2022

Conversation

xxchan
Copy link
Member

@xxchan xxchan commented Jun 6, 2022

What's changed and what's your intention?

As title. Mainly added ScanRange, which is built in LogicalScan::to_batch and used in RowSeqScan for different types of scan.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

part of #2429

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 826 files.

Valid Invalid Ignored Fixed
824 1 1 0
Click to see the invalid file list
  • src/frontend/src/utils/scan_range.rs

xxchan and others added 2 commits June 7, 2022 11:50
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #3014 (a1ca321) into main (97cb014) will decrease coverage by 0.07%.
The diff coverage is 57.65%.

@@            Coverage Diff             @@
##             main    #3014      +/-   ##
==========================================
- Coverage   73.48%   73.40%   -0.08%     
==========================================
  Files         733      734       +1     
  Lines       99548    99973     +425     
==========================================
+ Hits        73157    73390     +233     
- Misses      26391    26583     +192     
Flag Coverage Δ
rust 73.40% <57.65%> (-0.08%) ⬇️

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

Impacted Files Coverage Δ
src/common/src/util/ordered/serde.rs 91.06% <0.00%> (-1.98%) ⬇️
src/frontend/src/utils/mod.rs 66.66% <ø> (ø)
src/batch/src/executor/row_seq_scan.rs 23.27% <3.27%> (-20.28%) ⬇️
src/storage/src/table/cell_based_table.rs 72.97% <4.08%> (-17.73%) ⬇️
src/frontend/src/expr/mod.rs 86.40% <61.53%> (-5.81%) ⬇️
src/frontend/src/utils/scan_range.rs 80.00% <80.00%> (ø)
src/frontend/src/utils/condition.rs 93.59% <80.76%> (-5.28%) ⬇️
...frontend/src/optimizer/plan_node/batch_seq_scan.rs 95.76% <98.73%> (+3.30%) ⬆️
src/common/src/catalog/physical_table.rs 30.00% <100.00%> (+30.00%) ⬆️
...c/frontend/src/optimizer/plan_node/logical_scan.rs 100.00% <100.00%> (+0.37%) ⬆️
... and 4 more

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

@xxchan xxchan marked this pull request as ready for review June 7, 2022 16:07
@wcy-fdu
Copy link
Contributor

wcy-fdu commented Jun 8, 2022

#3008 merged.

@BugenZhao
Copy link
Member

Should we make PointGet a separate executor? 🤔

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 833 files.

Valid Invalid Ignored Fixed
831 1 1 0
Click to see the invalid file list
  • src/batch/src/executor/row_seq_scan.rs

@risingwavelabs risingwavelabs deleted a comment from github-actions bot Jun 8, 2022
@risingwavelabs risingwavelabs deleted a comment from github-actions bot Jun 8, 2022
@xxchan
Copy link
Member Author

xxchan commented Jun 8, 2022

Should we make PointGet a separate executor? 🤔

Why bother? 🤔

proto/batch_plan.proto Outdated Show resolved Hide resolved
@@ -436,7 +442,7 @@ async fn test_row_seq_scan() -> Result<()> {

let executor = Box::new(RowSeqScanExecutor::new(
table.schema().clone(),
table.iter_with_pk(u64::MAX, pk_descs).await.unwrap(),
ScanType::TableScan(table.iter_with_pk(u64::MAX, pk_descs).await.unwrap()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for other ScanType?

Copy link
Member Author

Choose a reason for hiding this comment

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

e2e test is enough?

pub eq_conds: Vec<Literal>,
/// ((lb, inclusive), (ub, inclusive))
#[allow(clippy::type_complexity)]
pub range: Option<(Option<(Literal, bool)>, Option<(Literal, bool)>)>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to change it to T: Range<Literal>

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@xxchan
Copy link
Member Author

xxchan commented Jun 8, 2022

😕
thread 'tokio-runtime-worker' panicked at 'not implemented: excluded begin key is not supported', /Users/xxchan/Projects/risingwave/src/storage/src/hummock/iterator/forward_user.rs:221:28

@BugenZhao Any idea how to fix it?

@xxchan
Copy link
Member Author

xxchan commented Jun 8, 2022

And Unbounded in storage not handled correctly 😕

@xxchan
Copy link
Member Author

xxchan commented Jun 8, 2022

finished except excluded start key 😇

@xxchan xxchan requested review from fuyufjh, skyzh and st1page June 8, 2022 09:57
Copy link
Contributor

@fuyufjh fuyufjh left a comment

Choose a reason for hiding this comment

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

LGTM!

pk_prefix: &Row,
next_col_bound: Bound<&Datum>,
is_start_bound: bool,
) -> StorageResult<Bound<Vec<u8>>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

So it seems unnecessary to return StorageResult<...>?

Copy link
Member Author

Choose a reason for hiding this comment

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

serialize_pk returns Result

Copy link
Contributor

Choose a reason for hiding this comment

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

I think serialize_pk should not return StorageResult as well...

Copy link
Member Author

@xxchan xxchan Jun 9, 2022

Choose a reason for hiding this comment

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

Yes it returns Ok 😇 Let's get rid of it

@skyzh
Copy link
Contributor

skyzh commented Jun 8, 2022

😕 thread 'tokio-runtime-worker' panicked at 'not implemented: excluded begin key is not supported', /Users/xxchan/Projects/risingwave/src/storage/src/hummock/iterator/forward_user.rs:221:28

@BugenZhao Any idea how to fix it?

Currently we only implemented some of the bound. If you need exclude bound, use next_key to convert it into an include bound.

included end key -> excluded next key
@xxchan
Copy link
Member Author

xxchan commented Jun 8, 2022

If you need exclude bound, use next_key to convert it into an include bound.

Ok I did it. Btw I also found I need to convert included end key -> excluded next key 😇

cc @wcy-fdu Maybe #3008 also needs some fix? I'm not sure

@xxchan
Copy link
Member Author

xxchan commented Jun 8, 2022

image

No idea why it fails 😇

Oh my local machine, ./risedev slt -p 4566 'e2e_test/batch/range_scan.slt' -d dev with ci-3cn-1fe also fails, but then I can see the correct result using psql.

@xxchan
Copy link
Member Author

xxchan commented Jun 8, 2022

I feel confused about whether and where to add wait_epoch 😇

@xxchan xxchan merged commit 0e313b1 into main Jun 9, 2022
@xxchan xxchan deleted the xxchan/point branch June 9, 2022 01:02
@hengm3467
Copy link
Contributor

@xxchan Does this PR contain user-facing changes? Thanks.

@xxchan
Copy link
Member Author

xxchan commented Jul 11, 2022

@xxchan Does this PR contain user-facing changes? Thanks.

This is a perfermance optimization. No behavior changes.

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

7 participants