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.

Sign up for GitHub

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

Jump to bottom

fix: 1554 lag correctness #1605

Merged
merged 9 commits into from
May 6, 2022
Merged

fix: 1554 lag correctness #1605

merged 9 commits into from
May 6, 2022

Conversation

aceforeverd
Copy link
Collaborator

@aceforeverd aceforeverd commented Apr 10, 2022 •
edited
Loading

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

close #1554

for those where lag function exists in sql, e.g

select id, sum(col) over w1, lag(col, 0) over w1 from t1 window w1 as (....)

the logic plan is constructed, previous:

Query
  ProjectPlan
    project_vec
      -ProjectList 1
        - id
        - sum(col)
        - lag(col, 0)

after the PR:

add a restriction to lag's offset: it must be constant

the lag project node will splited into seperate project list if:

  • lag over a rows_range window

new window is a ROWS type window:

  • without max_size
  • window start = offset
  • window end = current row

This is a internal split that happens before window merge, so mergable window can still merged based on original logic

@aceforeverd aceforeverd requested review from zhanghaohit and jingchen2222 and removed request for zhanghaohit April 10, 2022 14:28
@github-actions
Copy link
Contributor

github-actions bot commented Apr 10, 2022 •
edited
Loading

SDK Test Report

  73 files    73 suites   7m 49s ⏱️
174 tests 171 ✔️ 3 💤 0 ❌
215 runs  212 ✔️ 3 💤 0 ❌

Results for commit 3d4a879.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 10, 2022 •
edited
Loading

HybridSE Linux Test Report

       67 files       235 suites   6m 12s ⏱️
19 003 tests 19 001 ✔️ 2 💤 0 ❌

Results for commit 3d4a879.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 10, 2022 •
edited
Loading

HybridSE Mac Test Report

       67 files       235 suites   6m 27s ⏱️
19 003 tests 19 001 ✔️ 2 💤 0 ❌

Results for commit 3d4a879.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 10, 2022 •
edited
Loading

Linux Test Report

       57 files       186 suites   1h 8m 6s ⏱️
  8 467 tests   8 459 ✔️ 8 💤 0 ❌
12 418 runs  12 410 ✔️ 8 💤 0 ❌

Results for commit 3d4a879.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Apr 10, 2022 •
edited
Loading

Codecov Report

Merging #1605 (3d4a879) into main (d3067e4) will increase coverage by 0.07%.
The diff coverage is 96.66%.

@@             Coverage Diff              @@
##               main    #1605      +/-   ##
============================================
+ Coverage     73.58%   73.65%   +0.07%     
  Complexity      347      347              
============================================
  Files           614      614              
  Lines        119908   119984      +76     
  Branches       1010     1010              
============================================
+ Hits          88233    88373     +140     
+ Misses        31466    31402      -64     
  Partials        209      209
Impacted Files Coverage Δ
hybridse/include/node/node_manager.h 100.00% <ø> (ø)
...src/passes/expression/window_iter_analysis_test.cc 100.00% <ø> (ø)
hybridse/src/plan/planner.h 100.00% <ø> (ø)
hybridse/src/node/sql_node.cc 80.86% <71.42%> (+3.72%) ⬆️
hybridse/src/plan/planner.cc 93.50% <95.45%> (+0.30%) ⬆️
hybridse/include/codec/list_iterator_codec.h 80.74% <100.00%> (ø)
hybridse/include/node/plan_node.h 96.78% <100.00%> (ø)
hybridse/include/node/sql_node.h 83.30% <100.00%> (+1.54%) ⬆️
hybridse/src/node/node_manager.cc 89.24% <100.00%> (-0.02%) ⬇️
hybridse/src/node/plan_node.cc 85.00% <100.00%> (-0.03%) ⬇️
... and 26 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 d3067e4...3d4a879. Read the comment docs.

@aceforeverd
Copy link
Collaborator Author

High-priority Bugs

@aceforeverd aceforeverd mentioned this pull request Apr 11, 2022
Closed
3 tasks
@aceforeverd
Copy link
Collaborator Author

TODO: discuss TTL cal in deploy progress

zhanghaohit
zhanghaohit reviewed Apr 12, 2022
View reviewed changes
hybridse/src/plan/planner.cc Outdated Show resolved Hide resolved
hybridse/src/planv2/planner_v2_test.cc Show resolved Hide resolved
tobegit3hub
tobegit3hub approved these changes Apr 13, 2022
View reviewed changes
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

@aceforeverd
Copy link
Collaborator Author

Don't merge util performance issue resolved

@zhanghaohit zhanghaohit self-requested a review April 13, 2022 03:40
zhanghaohit
zhanghaohit previously requested changes Apr 13, 2022
View reviewed changes
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.

performance issue of unbound window frame

@aceforeverd aceforeverd force-pushed the fix/1554-multi-window-query-result branch from 24f7242 to 20b3265 Compare April 27, 2022 08:41
@github-actions github-actions bot added build openmldb compiling and installing execute-engine hybridse sql engine labels Apr 27, 2022
@aceforeverd aceforeverd requested a review from zhanghaohit April 27, 2022 08:57
zhanghaohit
zhanghaohit approved these changes May 5, 2022
View reviewed changes
hybridse/src/plan/planner.cc Outdated Show resolved Hide resolved
aceforeverd added 3 commits May 5, 2022 22:47
@aceforeverd
add a few debug log for sql parser
3275a1b
@aceforeverd
fix(4paradigm#1554): lag results always evaluated with respect to cur…
df949cb 
…rent row

- logic plan: for lag/at project, it will create a new `ProjectListNode`
  with window frame bound to [unbound, current row]
- the fix may not work in batch-request or cluster environment
@aceforeverd
fix planner_v2_test
332fba3
aceforeverd added 6 commits May 5, 2022 22:47
@aceforeverd
fix uts
63f4ff4
@aceforeverd
re-fix 4paradigm#1554
54874fc 
it will create a new rows window for lag like functions
@aceforeverd
revert change
77e44ee
@aceforeverd
test: fix window_iter_analysis_test
c685f61 
we don't allow lag offset as a udf call
@aceforeverd
improve logic
9ae9240
@aceforeverd
fix by comment
3d4a879
@aceforeverd aceforeverd force-pushed the fix/1554-multi-window-query-result branch from b5befb6 to 3d4a879 Compare May 5, 2022 14:49
@aceforeverd aceforeverd merged commit 346092e into 4paradigm:main May 6, 2022
@aceforeverd aceforeverd deleted the fix/1554-multi-window-query-result branch May 6, 2022 06:33
@aceforeverd aceforeverd mentioned this pull request May 12, 2022
Closed
@lumianph lumianph mentioned this pull request May 13, 2022
Closed
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhanghaohit zhanghaohit zhanghaohit approved these changes

@tobegit3hub tobegit3hub tobegit3hub approved these changes

@jingchen2222 jingchen2222 Awaiting requested review from jingchen2222

Assignees

@aceforeverd aceforeverd

Labels
build openmldb compiling and installing execute-engine hybridse sql engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lag()/at()/lead() return show offset'th row, it is not related to window frame bound
3 participants
@aceforeverd @tobegit3hub @zhanghaohit