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(#2211): disable window merge for rows/rows_range window if both #2278

Merged

Conversation

aceforeverd
Copy link
Collaborator

@aceforeverd aceforeverd commented Jul 31, 2022

close #2211

Root cause

rows window and rows_range window can be merged in a query, result in a inner only window type ( RowsMergeRowsRange window), this type of window for exclude current_row is not handled in physical plan, neither by codegen or runner.

Solution

I'm not going to fix it in runner and codegen yet, so this resolve the correctness issue by disable window merge for the too window, if both set exclude current_row

@github-actions github-actions bot added the execute-engine hybridse sql engine label Jul 31, 2022
@codecov
Copy link

codecov bot commented Jul 31, 2022

Codecov Report

Merging #2278 (14333a2) into main (6e691e8) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #2278      +/-   ##
============================================
- Coverage     75.75%   75.74%   -0.02%     
  Complexity      367      367              
============================================
  Files           624      624              
  Lines        118073   118077       +4     
  Branches       1048     1048              
============================================
- Hits          89446    89436      -10     
- Misses        28410    28424      +14     
  Partials        217      217              
Impacted Files Coverage Δ
hybridse/src/node/sql_node.cc 80.11% <100.00%> (+0.04%) ⬆️
...s/query_response_time/deploy_query_response_time.h 81.25% <0.00%> (-18.75%) ⬇️
src/storage/disk_table_snapshot.cc 56.71% <0.00%> (-4.48%) ⬇️
...atistics/query_response_time/query_response_time.h 78.26% <0.00%> (-4.35%) ⬇️
.../query_response_time/deploy_query_response_time.cc 97.10% <0.00%> (-2.90%) ⬇️
src/sdk/db_sdk.cc 65.69% <0.00%> (-0.90%) ⬇️
src/catalog/client_manager.cc 42.20% <0.00%> (-0.29%) ⬇️
src/nameserver/name_server_impl.cc 42.61% <0.00%> (-0.21%) ⬇️
src/client/ns_client.cc 46.94% <0.00%> (-0.15%) ⬇️
hybridse/src/vm/runner.cc 67.74% <0.00%> (-0.04%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e691e8...14333a2. Read the comment docs.

@github-actions
Copy link
Contributor

SDK Test Report

  77 files    77 suites   5m 53s ⏱️
166 tests 164 ✔️ 2 💤 0
208 runs  206 ✔️ 2 💤 0

Results for commit 14333a2.

@github-actions
Copy link
Contributor

HybridSE Linux Test Report

19 290 tests   19 288 ✔️  5m 41s ⏱️
     237 suites           2 💤
       67 files             0

Results for commit 14333a2.

@github-actions
Copy link
Contributor

HybridSE Mac Test Report

19 290 tests   19 288 ✔️  6m 45s ⏱️
     237 suites           2 💤
       67 files             0

Results for commit 14333a2.

@github-actions
Copy link
Contributor

Linux Test Report

       55 files       200 suites   53m 48s ⏱️
  9 254 tests   9 250 ✔️ 4 💤 0
13 615 runs  13 611 ✔️ 4 💤 0

Results for commit 14333a2.

@aceforeverd
Copy link
Collaborator Author

exclude current row

@aceforeverd aceforeverd merged commit d0db545 into 4paradigm:main Aug 1, 2022
@aceforeverd aceforeverd deleted the fix-2211-exclude-current-row branch August 28, 2022 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execute-engine hybridse sql engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incorrect result exclude current_row If two windows are merged
3 participants