[Native] Add local-exchange.max-partition-buffer-size system config

[kagamiori]

Description

Expose the Velox max_local_exchange_partition_buffer_size query config as a system config local-exchange.max-partition-buffer-size to allow tuning.

Motivation and Context

Impact

Test Plan

Updated unit tests in BaseVeloxQueryConfigTest.cpp

Contributor checklist

• Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
• PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
• Documented new properties (with its default value), SQL syntax, functions, or other functionality.
• If release notes are required, they follow the release notes guidelines.
• Adequate tests were added if applicable.
• CI passed.

Release Notes

== NO RELEASE NOTE ==

[steveburnett]

If this session property is being added to Prestissimo, then it should probably be added to the Presto C++ Session Properties documentation.

[kagamiori]

If this session property is being added to Prestissimo, then it should probably be added to the Presto C++ Session Properties documentation.

Hi @steveburnett, this PR doesn't add max_local_exchange_partition_buffer_size as a session property. It makes this property tunable through the 'velox.properties' config file. I've updated the PR title and summary to be more accurate. (Please see bafb615 about the velox.properties config file.)

[aditi-pandit]

@kagamiori : We are generally not bringing new config from velox.properties these days and prefer to get from SystemConfig. Is it possible to do the same with this property as well ?

[kagamiori]

@kagamiori : We are generally not bringing new config from velox.properties these days and prefer to get from SystemConfig. Is it possible to do the same with this property as well ?

Hi @aditi-pandit, I've updated it as a system config instead of velox.properties according to @kewang1024's suggestion.

[aditi-pandit]

Thanks @kagamiori. Please get review from @steveburnett as well.

[kewang1024]

Thanks for changing to using SystemConfig @kagamiori

[kagamiori]

If this session property is being added to Prestissimo, then it should probably be added to the Presto C++ Session Properties documentation.

Hi @steveburnett, I've updated this PR to make the new config a system config, and updated the presto-docs/src/main/sphinx/presto_cpp/properties.rst file. Thank you for the suggestion!

[kewang1024]

Sorry, just took another look, since we're deprecating velox.properties and only use config.properties, we shouldn't use updateLoadedValues from BaseVeloxQueryConfig, instead we should use updateFromSystemConfigs

For testing, consider add an equivalent of BaseVeloxQueryConfigTest for SystemConfig

[kewang1024]

Move this to

presto/presto-native-execution/presto_cpp/main/QueryContextManager.cpp

Line 42 in a31cccf

void updateFromSystemConfigs(

[kagamiori]

Hi @kewang1024, uint tests in QueryContextManagerTest.cpp only update session properties using protocol::SessionRepresentation. How should I add a test case that updates system config there?

[kewang1024]

You don't need the test on overriding session property

QueryContextManager will do the following

• get base map<string, string> from BaseVeloxQueryConfig (which comes from velox.properties which we will deprecate)
• update the session property value
• updateFromSystemConfigs

[kewang1024]

Thank you! @kagamiori

[kagamiori]

Hi @steveburnett, I've updated the document. Could you take another look? Thanks!