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(engine): limt clause conjunction with where clause or group by clause #2447

Merged
merged 12 commits into from
Sep 9, 2022

Conversation

aceforeverd
Copy link
Collaborator

@aceforeverd aceforeverd commented Sep 7, 2022

fixes:

  • limit of where clause
  • limit of group by (+ having) clause
  • correctness of limit 0: should returns empty list

close #2433

the pr introduce a new iterator (LimitIterator) used for limit clause conjunction with where clause. The iterator is not seekable, so window project over this is not supported as well, we need the compile time check for this.

some restrictions should noted:

  • limit clause do not support in request mode, I will add compile check later
  • by practice, limit clause should used together with order by, while we do not support order by yet. As a result, limit result may not deterministic

@github-actions github-actions bot added execute-engine hybridse sql engine storage-engine openmldb storage engine. nameserver & tablet labels Sep 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

SDK Test Report

  81 files    81 suites   7m 51s ⏱️
168 tests 166 ✔️ 2 💤 0
210 runs  208 ✔️ 2 💤 0

Results for commit a011b2e.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

Linux Test Report

       55 files       203 suites   1h 4m 0s ⏱️
  9 903 tests   9 896 ✔️ 7 💤 0
14 579 runs  14 572 ✔️ 7 💤 0

Results for commit a011b2e.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

HybridSE Linux Test Report

19 792 tests   19 790 ✔️  7m 11s ⏱️
     239 suites           2 💤
       67 files             0

Results for commit a011b2e.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

HybridSE Mac Test Report

19 792 tests   19 790 ✔️  7m 34s ⏱️
     239 suites           2 💤
       67 files             0

Results for commit a011b2e.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Base: 75.90% // Head: 75.79% // Decreases project coverage by -0.11% ⚠️

Coverage data is based on head (a011b2e) compared to base (6175266).
Patch coverage: 72.95% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2447      +/-   ##
============================================
- Coverage     75.90%   75.79%   -0.12%     
  Complexity      368      368              
============================================
  Files           629      638       +9     
  Lines        119694   119939     +245     
  Branches       1044     1056      +12     
============================================
+ Hits          90859    90907      +48     
- Misses        28619    28816     +197     
  Partials        216      216              
Impacted Files Coverage Δ
hybridse/src/passes/physical/limit_optimized.h 100.00% <ø> (ø)
hybridse/src/vm/catalog_wrapper.h 47.60% <51.72%> (+2.34%) ⬆️
hybridse/src/passes/physical/limit_optimized.cc 75.00% <75.00%> (ø)
hybridse/src/vm/physical_op.cc 79.82% <84.21%> (+0.29%) ⬆️
hybridse/src/vm/runner.cc 68.14% <95.23%> (+0.04%) ⬆️
hybridse/include/vm/physical_op.h 91.85% <100.00%> (ø)
...src/passes/physical/batch_request_optimize_test.cc 100.00% <100.00%> (ø)
hybridse/src/testing/engine_test_base.cc 80.64% <100.00%> (+0.04%) ⬆️
hybridse/src/vm/runner.h 92.86% <100.00%> (-0.02%) ⬇️
...radigm/openmldb/batch/nodes/DataProviderPlan.scala 62.50% <100.00%> (ø)
... and 30 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@tobegit3hub tobegit3hub left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the batch-engine openmldb batch(offline) engine label Sep 8, 2022
@aceforeverd
Copy link
Collaborator Author

the pr also correct behavior of limit 0, @tobegit3hub pls recheck it :)

Copy link
Collaborator

@zhanghaohit zhanghaohit left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@tobegit3hub tobegit3hub left a comment

Choose a reason for hiding this comment

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

LGTM

@tobegit3hub tobegit3hub merged commit fdfd1f2 into 4paradigm:main Sep 9, 2022
@aceforeverd aceforeverd deleted the fix-2433 branch September 9, 2022 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
batch-engine openmldb batch(offline) engine execute-engine hybridse sql engine storage-engine openmldb storage engine. nameserver & tablet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

where xxx = xxx limit 1返回多条数据
3 participants