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

fix(streaming): ensure schema and order are consistent in lookup #1970

Merged
merged 4 commits into from
Apr 20, 2022

Conversation

skyzh
Copy link
Contributor

@skyzh skyzh commented Apr 20, 2022

Signed-off-by: Alex Chi iskyzh@gmail.com

What's changed and what's your intention?

Currently, all TPC-H can pass under delta join. (Only tested single node).

image

This PR fixes a lot of bugs. Now we ensures that:

  • Lookup will output stream + arrangement internally, and then reordered by column reordering parameters.
  • Lookup's input must be stream then arrangement.

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)

blocked by #1901, causing MV with delta join cannot be dropped.
close #1882

@codecov
Copy link

codecov bot commented Apr 20, 2022

Codecov Report

Merging #1970 (dc67923) into main (90b908a) will increase coverage by 0.02%.
The diff coverage is 75.91%.

@@            Coverage Diff             @@
##             main    #1970      +/-   ##
==========================================
+ Coverage   70.98%   71.00%   +0.02%     
==========================================
  Files         624      624              
  Lines       80480    80557      +77     
==========================================
+ Hits        57126    57198      +72     
- Misses      23354    23359       +5     
Flag Coverage Δ
rust 71.00% <75.91%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
src/meta/src/stream/rewrite/delta_join.rs 0.00% <0.00%> (ø)
src/stream/src/executor_v2/lookup.rs 4.83% <0.00%> (-0.08%) ⬇️
src/stream/src/executor_v2/lookup/sides.rs 29.16% <ø> (ø)
src/meta/src/stream/fragmenter.rs 85.01% <25.00%> (+0.74%) ⬆️
src/stream/src/executor_v2/lookup/impl_.rs 96.77% <93.65%> (-0.69%) ⬇️
src/common/src/array/data_chunk.rs 94.97% <100.00%> (+0.12%) ⬆️
src/common/src/array/stream_chunk.rs 89.41% <100.00%> (+0.34%) ⬆️
src/stream/src/executor_v2/lookup/tests.rs 100.00% <100.00%> (ø)
src/common/src/types/ordered_float.rs 23.70% <0.00%> (+0.19%) ⬆️
... and 1 more

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

@skyzh
Copy link
Contributor Author

skyzh commented Apr 20, 2022

also hold until #1967? Modification on the schema part will easily break some schema checks. Should wait until there's a schema check executor so that we can be sure that there's no schema mismatch problems.

@BugenZhao
Copy link
Member

also hold until #1967? Modification on the schema part will easily break some schema checks. Should wait until there's a schema check executor so that we can be sure that there's no schema mismatch problems.

Good idea. I believe it will be quick.

@skyzh skyzh changed the title fix(streaming): ensure schema and order are consistent fix(streaming): ensure schema and order are consistent in lookup Apr 20, 2022
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.

By the way, IIUC, there is no e2e test for looking up shared state now?

@fuyufjh
Copy link
Contributor

fuyufjh commented Apr 20, 2022

By the way, IIUC, there is no e2e test for looking up shared state now?

Oh it's covered in #1978

Signed-off-by: Alex Chi <iskyzh@gmail.com>
Signed-off-by: Alex Chi <iskyzh@gmail.com>
Signed-off-by: Alex Chi <iskyzh@gmail.com>
@yuhao-su
Copy link
Contributor

LGTM

@skyzh skyzh enabled auto-merge (squash) April 20, 2022 10:15
@skyzh skyzh disabled auto-merge April 20, 2022 10:16
@skyzh skyzh enabled auto-merge (squash) April 20, 2022 10:16
@skyzh skyzh merged commit 30ee6e9 into main Apr 20, 2022
@skyzh skyzh deleted the skyzh/lookup-fix branch April 20, 2022 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

streaming: support column reordering in lookup executor
4 participants