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

[native] Add partitioned output buffers sessions #23853

Merged
merged 1 commit into from
Oct 21, 2024
Merged

[native] Add partitioned output buffers sessions #23853

merged 1 commit into from
Oct 21, 2024

Conversation

tanjialiang
Copy link
Contributor

@tanjialiang tanjialiang commented Oct 17, 2024 •
edited
Loading

Added 2 native session properties to control partitioned output buffers

== RELEASE NOTES ==

Session changes
- Add session property: 'native_max_page_partitioning_buffer_size' :pr:`23853`
- Add session property: 'native_max_output_buffer_size' :pr:`23853`

@tanjialiang tanjialiang requested review from a team as code owners October 17, 2024 19:00
@tanjialiang tanjialiang force-pushed the po_session branch 2 times, most recently from f7eed9b to 54b561d Compare October 17, 2024 22:52
swapsmagic
swapsmagic reviewed Oct 17, 2024
View reviewed changes
presto-native-execution/presto_cpp/main/SessionProperties.cpp
@@ -299,11 +299,32 @@ SessionProperties::SessionProperties() {
kQueryTraceTaskRegExp,
"The regexp of traced task id. We only enable trace on a task if its id"
" matches.",
BIGINT(),
VARCHAR(),
Copy link
Contributor

Choose a reason for hiding this comment

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

was that an existing bug that we are fixing along with introducing new session properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that was a bug..

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to add some release note around the bug fix? Or its minor enough that no release note needed?

swapsmagic
swapsmagic previously approved these changes Oct 17, 2024
View reviewed changes
NikhilCollooru
NikhilCollooru reviewed Oct 17, 2024
View reviewed changes
presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java
Comment on lines +1816 to +1818
"SerializedPages. For PartitionedOutputNode::Kind::kPartitioned, PartitionedOutput operator " +
"would buffer up to that number of bytes / number of destinations for each destination before " +
"producing a SerializedPage.",
Copy link
Contributor

Choose a reason for hiding this comment

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

align on left

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The checkstyle does not seem to like it when I tried to align the concatenated strings left..

NikhilCollooru
NikhilCollooru previously approved these changes Oct 17, 2024
View reviewed changes
@steveburnett
Copy link
Contributor

Consider adding documentation for the new session properties to https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/presto_cpp/properties-session.rst

@tdcmeehan tdcmeehan self-assigned this Oct 18, 2024
@tdcmeehan
Copy link
Contributor

LGTM, but let's add the doc for this session property--this is part of the contributor checklist on each PR.

steveburnett
steveburnett approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull branch, local docs build, looks good. Thanks!

@steveburnett
Copy link
Contributor

Nit, include the PR number in the release note entry, and rephrase to active voice for consistency with the Order of changes in the Release Notes Guidelines, like this:

== RELEASE NOTES ==

Session changes
- Add session property 'native_max_page_partitioning_buffer_size' :pr:`23853`
- Add session property 'native_max_output_buffer_size' :pr:`23853`

@tanjialiang tanjialiang merged commit 38e977e into prestodb:master Oct 21, 2024
60 checks passed
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
Merged
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NikhilCollooru NikhilCollooru NikhilCollooru approved these changes

@steveburnett steveburnett steveburnett approved these changes

@gggrace14 gggrace14 gggrace14 approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@elharo elharo Awaiting requested review from elharo elharo is a code owner

@swapsmagic swapsmagic Awaiting requested review from swapsmagic

Assignees

@tdcmeehan tdcmeehan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tanjialiang @steveburnett @tdcmeehan @gggrace14 @swapsmagic @NikhilCollooru