Skip to content

Comments

[native] Add partitioned output buffers sessions#23853

Merged
tanjialiang merged 1 commit intoprestodb:masterfrom
tanjialiang:po_session
Oct 21, 2024
Merged

[native] Add partitioned output buffers sessions#23853
tanjialiang merged 1 commit intoprestodb:masterfrom
tanjialiang:po_session

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
"The regexp of traced task id. We only enable trace on a task if its id"
" matches.",
BIGINT(),
VARCHAR(),
Copy link
Contributor

@swapsmagic swapsmagic Oct 17, 2024

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

@tanjialiang tanjialiang Oct 17, 2024

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

@swapsmagic swapsmagic Oct 18, 2024

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

@NikhilCollooru NikhilCollooru Oct 17, 2024

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

@tanjialiang tanjialiang Oct 18, 2024

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

steveburnett commented Oct 18, 2024

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

tdcmeehan commented Oct 18, 2024

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

@tanjialiang tanjialiang dismissed stale reviews from NikhilCollooru and swapsmagic via 6bf712f October 19, 2024 21:59
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

steveburnett commented Oct 21, 2024

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
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
Merged
25 tasks
@tdcmeehan tdcmeehan added the from:Meta PR from Meta label Dec 13, 2024
@prestodb-ci
Copy link
Contributor

prestodb-ci commented Dec 13, 2024

Saved that user @tanjialiang is from Meta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gggrace14 gggrace14 gggrace14 approved these changes

@steveburnett steveburnett steveburnett approved these changes

@NikhilCollooru NikhilCollooru NikhilCollooru 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

from:Meta PR from Meta

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tanjialiang @steveburnett @tdcmeehan @prestodb-ci @gggrace14 @swapsmagic @NikhilCollooru