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

executor: do not return first row until the frame is completed. #12480

Merged
merged 8 commits into from
Oct 12, 2019

Conversation

SunRunAway
Copy link
Contributor

@SunRunAway SunRunAway commented Sep 29, 2019

What problem does this PR solve?

Fixes #12415.

What is changed and how it works?

In previous implementation, windowExec returns a chunk when a frame is fetched but not evaluated. After rewriting, windowExec returns a chunk after a frame is finished evaluating.

It makes the code simpler, and all the implementation of aggregation functions can reference the data in the frame. The bad side is it will increase the time of meeting first row.

I'll send a following PR(#12481) to revert the changes of copying data in #11678, if this is accepted by reviewers.

Check List

Tests

Code changes

  • None

Side effects

  • It will increase the time of meeting first row

Related changes

  • None

Release note

  • Fix a memory reference bug in window function.

@SunRunAway SunRunAway added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Sep 29, 2019
@codecov
Copy link

codecov bot commented Sep 29, 2019

Codecov Report

Merging #12480 into master will decrease coverage by 0.0778%.
The diff coverage is 83.6363%.

@@               Coverage Diff                @@
##             master     #12480        +/-   ##
================================================
- Coverage   79.8678%   79.7899%   -0.0779%     
================================================
  Files           461        460         -1     
  Lines        103809     102172      -1637     
================================================
- Hits          82910      81523      -1387     
+ Misses        14809      14761        -48     
+ Partials       6090       5888       -202

@SunRunAway
Copy link
Contributor Author

/run-all-tests

@SunRunAway
Copy link
Contributor Author

/run-all-tests

@SunRunAway
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Oct 1, 2019

@zz-jason, @qw4990, @lamxTyler, PTAL.

2 similar comments
@sre-bot
Copy link
Contributor

sre-bot commented Oct 3, 2019

@zz-jason, @qw4990, @lamxTyler, PTAL.

@sre-bot
Copy link
Contributor

sre-bot commented Oct 6, 2019

@zz-jason, @qw4990, @lamxTyler, PTAL.

executor/window_test.go Show resolved Hide resolved
executor/window.go Outdated Show resolved Hide resolved
executor/window.go Outdated Show resolved Hide resolved
executor/window.go Outdated Show resolved Hide resolved
executor/window.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@sre-bot
Copy link
Contributor

sre-bot commented Oct 11, 2019

@lamxTyler, @zz-jason, @qw4990, @XuHuaiyu, PTAL.

executor/window.go Show resolved Hide resolved
executor/window.go Outdated Show resolved Hide resolved
Co-Authored-By: Zhuomin(Charming) Liu <lzmhhh123@gmail.com>
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

@SunRunAway Pls fix the ci, then LGTM

@lzmhhh123 lzmhhh123 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Oct 12, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 12, 2019

/run-all-tests

@sre-bot sre-bot merged commit 03addcb into pingcap:master Oct 12, 2019
@SunRunAway SunRunAway deleted the window-func branch October 12, 2019 11:38
@sre-bot
Copy link
Contributor

sre-bot commented Oct 12, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Oct 12, 2019

cherry pick to release-3.1 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

window function panic: runtime error: index out of range
4 participants