Skip to content

Comments

feat(sql): window without order by#3554

Merged
aceforeverd merged 1 commit into4paradigm:mainfrom
aceforeverd:feat-window-without-order-by
Nov 9, 2023
Merged

feat(sql): window without order by#3554
aceforeverd merged 1 commit into4paradigm:mainfrom
aceforeverd:feat-window-without-order-by

Conversation

@aceforeverd
Copy link
Collaborator

@aceforeverd aceforeverd commented Oct 13, 2023

Rules:

  • ALLOWED for ROWS-type WINDOW
  • NOT ALLOWED for RANGE-type WINDOW with offset PRECEDING/FOLLOWING frame
  • NOT ALLOWED for WINDOW with attribute EXCLUDE CURRENT_TIME

Usage notice for WINDOW without ORDER BY clause:

  • Without ORDER BY, rows are processed in an unspecified order, neither which rows are going to included in the window or the ordering among rows
  • General-Purpose Window Functions, e.g lag, first_value, ..., depend on the sort ordering specified by the ORDER BY clause of the associated window definition. In which case, MAY returns undetermined result
  • For batch mode, the query result is undetermined, for request mode, the request row is ensured to be BEFORE OR THE LAST ROW from window frame

@github-actions github-actions bot added the execute-engine hybridse sql engine label Oct 13, 2023
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (024a0d0) 83.35% compared to head (b30b42d) 75.32%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@              Coverage Diff              @@
##               main    #3554       +/-   ##
=============================================
- Coverage     83.35%   75.32%    -8.03%     
- Complexity        0      587      +587     
=============================================
  Files            17      718      +701     
  Lines           751   129786   +129035     
  Branches          0     1280     +1280     
=============================================
+ Hits            626    97766    +97140     
- Misses          109    31734    +31625     
- Partials         16      286      +270     
Files Coverage Δ
hybridse/include/node/sql_node.h 84.93% <ø> (ø)
hybridse/src/node/sql_node.cc 76.80% <100.00%> (ø)
hybridse/src/plan/planner.cc 88.27% <ø> (ø)
hybridse/src/testing/engine_test_base.cc 80.49% <100.00%> (ø)
hybridse/src/vm/runner.cc 69.92% <100.00%> (ø)
hybridse/src/vm/transform.cc 83.92% <100.00%> (ø)
hybridse/include/vm/physical_op.h 91.03% <75.00%> (ø)

... and 694 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aceforeverd aceforeverd force-pushed the feat-window-without-order-by branch from 3fec6a9 to 208483e Compare October 14, 2023 02:45
@aceforeverd aceforeverd marked this pull request as ready for review October 14, 2023 02:46
@aceforeverd aceforeverd force-pushed the feat-window-without-order-by branch from 208483e to 4776940 Compare October 14, 2023 12:30
@lumianph lumianph requested a review from lqy222 October 16, 2023 03:28
aceforeverd added a commit to aceforeverd/fedb that referenced this pull request Oct 16, 2023
aceforeverd added a commit to aceforeverd/fedb that referenced this pull request Oct 16, 2023
Rules:
- ALLOWED for ROWS-type WINDOW
- NOT ALLOWED for RANGE-type WINDOW with offset PRECEDING/FOLLOWING
- NOT ALLOWED for WINDOW with attribute EXCLUDE CURRENT_TIME

Without ORDER BY, rows are processed in an unspecified order.
@aceforeverd aceforeverd force-pushed the feat-window-without-order-by branch from 4776940 to b30b42d Compare October 17, 2023 02:33
@aceforeverd
Copy link
Collaborator Author

TODO afterwards: fix auto-create-index logic during deployment, I don't cover that.

@aceforeverd aceforeverd merged commit 23d7c50 into 4paradigm:main Nov 9, 2023
@aceforeverd aceforeverd deleted the feat-window-without-order-by branch November 9, 2023 08:30
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.

3 participants