Skip to content

Implement Window Function Spilling#14909

Merged
highker merged 8 commits intoprestodb:masterfrom
sachdevs:windowspilling
Aug 4, 2020
Merged

Implement Window Function Spilling#14909
highker merged 8 commits intoprestodb:masterfrom
sachdevs:windowspilling

Conversation

@sachdevs
Copy link
Contributor

@sachdevs sachdevs commented Jul 28, 2020

Original PR: trinodb/trino#228
Broken up into ORDER BY spilling (merged in #14836) and Window function spilling (this one)

Part of #14935

TODO

  • Release notes
  • Window function docs

Saksham Sachdev and others added 8 commits July 28, 2020 09:17
Cherry-pick of trinodb/trino@17e3e09

Co-authored-by: Karol Sobczak <karol.sobczak@karolsobczak.com>
This avoids calling `Page.getRegion` on the input page and separates
input management from index building.

Cherry-pick of trinodb/trino@b164253

Co-authored-by: Karol Sobczak <karol.sobczak@karolsobczak.com>
Cherry-pick of trinodb/trino@7200a16

Co-authored-by: Atri Sharma <atri@linux.com>
Cherry-pick of trinodb/trino@d398d1c

Co-authored-by: Karol Sobczak <karol.sobczak@karolsobczak.com>
Cherry-pick of trinodb/trino@2b821e1

Co-authored-by: Karol Sobczak <karol.sobczak@karolsobczak.com>
Cherry-pick of trinodb/trino@d76a830

Co-authored-by: Karol Sobczak <karol.sobczak@karolsobczak.com>
Cherry-pick of trinodb/trino@fcc18ff

Co-authored-by: Karol Sobczak <karol.sobczak@karolsobczak.com>
@wenleix
Copy link
Contributor

wenleix commented Jul 28, 2020

@sachdevs : is it possible also to mention the PR in PrestoSQL that you backport from?

@sachdevs
Copy link
Contributor Author

Updated - it's the same as the ORDER BY spilling PR on prestoSQL's end.

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

"Port window operator to work processors" LGTM

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

"Separate input PageSource work processor" and "Introduce revocable aggregated memory context in OperatorContext" LGTM

@highker highker self-requested a review July 30, 2020 02:34
}

// caller shouldn't close this context as it's managed by the OperatorContext
public AggregatedMemoryContext aggregateRevocableMemoryContext()
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks quite general, surprised not added before when adding aggregation spilling

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

LGTM. @wenleix might be interested in taking another look

Copy link
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

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

First 3 commits LGTM:

  • Port window operator to work processors
  • Separate input PageSource work processor …
  • Introduce revocable aggregated memory context in OperatorContext

Copy link
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

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

"Implement spill to disk For WindowOperator". Generally looks good. Note SpillablePagesToPagesIndexes implements some kind of external sorting by index..

Optional<Page> currentSpillGroupRowPage;
Optional<Spiller> spiller;
ListenableFuture<?> spillInProgress = immediateFuture(null);
// Spill can be trigger by Driver, by us or both. `spillInProgress` is not empty when spill was triggered but not `finishMemoryRevoke()` yet
Copy link
Contributor

@wenleix wenleix Aug 3, 2020

Choose a reason for hiding this comment

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

I assume "us" here means this window operator itself, especially SpillablePagesToPagesIndexes#process

Copy link
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

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

"Use OrderingCompiler in WindowOperator spilling" . LGTM.

@highker highker merged commit 28b6062 into prestodb:master Aug 4, 2020
@caithagoras caithagoras mentioned this pull request Aug 14, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants