Spill for Window and OrderBy operators by sopel39 · Pull Request #228 · trinodb/trino

sopel39 · 2019-02-13T16:54:21Z

No description provided.

cla-bot · 2019-02-13T16:54:22Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Atri Sharma.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

sopel39 · 2019-02-13T16:56:52Z

@findepi new commits that require review:
Extract order by queries tests to separate class: 3dd2a44
Extract window queries tests to separate class: e0359db
Convert revocable memory to user memory on fully buffered group: 292a534
Use OrderingCompiler in WindowOperator spilling: ec46507
Convert revocable memory to user memory on OrderBy finish: ed8cc10
Use OrderingCompiler in OrderBy spilling: eb20fd9

New spill are enabled by default (when spilling is enabled).

cla-bot · 2019-02-14T06:59:13Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Atri Sharma.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveDistributedOrderByQueries.java

presto-tests/src/main/java/io/prestosql/tests/AbstractTestOrderByQueries.java

findepi

"Convert revocable memory to user memory on fully buffered group"

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveDistributedOrderByQueries.java

presto-main/src/main/java/io/prestosql/operator/WindowOperator.java

presto-main/src/test/java/io/prestosql/operator/TestWindowOperator.java

findepi · 2019-02-18T15:32:49Z

presto-main/src/main/java/io/prestosql/operator/WindowOperator.java

remove revocablePagesIndex parameter from updateMemoryUsage

How would we know if pages index is revocable then?

findepi

"Use OrderingCompiler in WindowOperator spilling"

presto-main/src/main/java/io/prestosql/operator/WindowOperator.java

findepi · 2019-02-18T15:35:51Z

presto-main/src/main/java/io/prestosql/operator/WindowOperator.java

i'd add this param after int preSortedChannelPrefix,, or after PagesIndex.Factory pagesIndexFactory,

findepi

"Convert revocable memory to user memory on OrderBy finish"

presto-main/src/main/java/io/prestosql/operator/OrderByOperator.java

presto-main/src/test/java/io/prestosql/operator/TestOrderByOperator.java

findepi

"Use OrderingCompiler in OrderBy spilling"

findepi · 2019-02-18T15:49:35Z

presto-main/src/main/java/io/prestosql/operator/OrderByOperator.java

move this param after List<SortOrder> sortOrder,

findepi · 2019-02-18T15:50:07Z

presto-main/src/main/java/io/prestosql/operator/OrderByOperator.java

move this field after private final List<SortOrder> sortOrder;

(and move private boolean closed; to be last, separated by a blank line)

I think that orderingCompiler is tightly coupled with spillerFactory, so they can as well be paired as parameters (similar in window operator)

Moved closed

findepi · 2019-02-18T15:50:53Z

presto-main/src/main/java/io/prestosql/operator/OrderByOperator.java

just ref-ing here 8284734#r257741196

findepi · 2019-02-18T15:53:28Z

@sopel39 i reviewed the new commits listed in #228 (comment)
Please apply changes as fixups (in order, no need to put the fixups at the end)

Also, please update Add Spill To Disk for ORDER BY commit message -- it says "This PR ..."

cla-bot · 2019-02-20T15:40:05Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Atri Sharma.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

sopel39

AC

presto-tests/src/main/java/io/prestosql/tests/AbstractTestOrderByQueries.java

sopel39 · 2019-02-20T15:28:07Z

presto-main/src/main/java/io/prestosql/operator/WindowOperator.java

How would we know if pages index is revocable then?

findepi

fixups LGTM

findepi · 2019-02-20T16:14:01Z

presto-main/src/main/java/io/prestosql/sql/gen/OrderingCompiler.java

Fix error message commit

Please squash this with the first commit which broadens the use of OrderingCompiler. Otherwise you lose the context of the "fix".

findepi

@sopel39 please

squash commits, including Fix error message commit
update Add Spill To Disk for ORDER BY commit message -- it says "This PR ..."

ORDER BY currently will error out if the data being processed exceeds query memory limit. This commit introduces paging from disk and ensures that ORDER BY is limited only by the amount of disk present.

cla-bot · 2019-02-21T10:51:11Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Atri Sharma.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

This avoids calling `Page.getRegion` on the input page and separates input management from index building.

cla-bot · 2019-02-21T10:52:15Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Atri Sharma.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

sopel39 · 2019-02-26T12:44:12Z

During the process of rebasing and preparing this PR to merge into master, some commits were squashed. We'd like to acknowledge that Atri Sharma (@atris) helped co-author these commits: 24868b3, d398d1c

sopel39 requested a review from findepi February 13, 2019 16:54

sopel39 force-pushed the epic/spill/1 branch from e0359db to e0ef8d3 Compare February 14, 2019 06:59

findepi reviewed Feb 18, 2019

View reviewed changes

presto-main/src/main/java/io/prestosql/operator/OrderByOperator.java Outdated Show resolved Hide resolved

presto-main/src/test/java/io/prestosql/operator/TestOrderByOperator.java Outdated Show resolved Hide resolved

findepi reviewed Feb 18, 2019

View reviewed changes

sopel39 commented Feb 20, 2019

View reviewed changes

findepi reviewed Feb 20, 2019

View reviewed changes

findepi approved these changes Feb 20, 2019

View reviewed changes

havi-odin and others added 4 commits February 21, 2019 11:46

Extract DummySpillerFactory from TestHashAggregationOperator

0db6dce

Use WorkProcessor in OrderByOperator

ff6dbe7

Add feature toggle for ORDER BY spill

9f2e005

Add Spill To Disk for ORDER BY

52fb5f6

ORDER BY currently will error out if the data being processed exceeds query memory limit. This commit introduces paging from disk and ensures that ORDER BY is limited only by the amount of disk present.

sopel39 force-pushed the epic/spill/1 branch from e2e3d24 to 37006b0 Compare February 21, 2019 10:51

sopel39 and others added 9 commits February 21, 2019 11:51

Use OrderingCompiler in OrderBy spilling

3ec17fe

Convert revocable memory to user memory on OrderBy finish

6b5c53e

Port window operator to work processors

5dd3d77

Separate input PageSource work processor

1804aec

This avoids calling `Page.getRegion` on the input page and separates input management from index building.

Introduce revocable aggregated memory context in OperatorContext

8c5abca

Add feature toggle for WindowOperator spill

debf505

Implement spill to disk For WindowOperator

63fbcba

Use OrderingCompiler in WindowOperator spilling

2946eb3

Convert revocable memory to user memory on fully buffered group

b35dfea

mattsfuller and others added 3 commits February 21, 2019 11:51

Add documentation for spill to disk for window and order by

b9fafc3

Extract order by queries tests to separate class

87a6657

Extract window queries tests to separate class

f30d3c6

sopel39 force-pushed the epic/spill/1 branch from 37006b0 to f30d3c6 Compare February 21, 2019 10:52

sopel39 merged commit fcc18ff into trinodb:master Feb 21, 2019

sopel39 deleted the epic/spill/1 branch February 21, 2019 11:06

sopel39 mentioned this pull request Jan 9, 2020

Add 2019 summary post trinodb/trino.io#66

Merged

sachdevs mentioned this pull request Jul 14, 2020

Implement ORDER BY spilling prestodb/presto#14836

Merged

sachdevs mentioned this pull request Jul 28, 2020

Implement Window Function Spilling prestodb/presto#14909

Merged

Conversation

sopel39 commented Feb 13, 2019

Uh oh!

cla-bot bot commented Feb 13, 2019

Uh oh!

sopel39 commented Feb 13, 2019

Uh oh!

cla-bot bot commented Feb 14, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Feb 18, 2019

Uh oh!

cla-bot bot commented Feb 20, 2019

Uh oh!

sopel39 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

cla-bot bot commented Feb 21, 2019

Uh oh!

cla-bot bot commented Feb 21, 2019

Uh oh!

sopel39 commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

sopel39 commented Feb 26, 2019 •

edited

Loading