Skip to content

[native] Add presto.default-namespace Prestissimo config property#23384

Closed
pdabre12 wants to merge 1 commit intoprestodb:masterfrom
pdabre12:add-default-namespace-config-property
Closed

[native] Add presto.default-namespace Prestissimo config property#23384
pdabre12 wants to merge 1 commit intoprestodb:masterfrom
pdabre12:add-default-namespace-config-property

Conversation

@pdabre12
Copy link
Contributor

@pdabre12 pdabre12 commented Aug 5, 2024 •
edited
Loading

Description

Adds the presto.default-namespace config property.

Resolves: #23385

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==
Prestissimo Changes
* Add ``presto.default-namespace`` Prestissimo configuration property :pr:`23384`

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 5, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: pdabre12 / name: Pratik Joseph Dabre (48710fd)

@steveburnett
Copy link
Contributor

steveburnett commented Aug 5, 2024

Suggestions:

== RELEASE NOTES ==

General Changes
* Add ``presto.default-namespace`` configuration property for Presto C++ :pr:`23384`

@pdabre12 pdabre12 force-pushed the add-default-namespace-config-property branch from b685c5a to 77bb53e Compare August 5, 2024 21:12
@pdabre12 pdabre12 changed the title Add presto.default-namespace prestissimo config property [native] Add presto.default-namespace prestissimo config property Aug 5, 2024
@pdabre12 pdabre12 force-pushed the add-default-namespace-config-property branch from 77bb53e to 807baa9 Compare August 5, 2024 21:15
@pdabre12 pdabre12 changed the title [native] Add presto.default-namespace prestissimo config property [native] Add presto.default-namespace Prestissimo config property Aug 5, 2024
steveburnett
steveburnett requested changes Aug 5, 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.

Thanks for the doc!

@pdabre12 pdabre12 force-pushed the add-default-namespace-config-property branch from 807baa9 to f63e801 Compare August 5, 2024 22:16
@pdabre12
Copy link
Contributor Author

pdabre12 commented Aug 5, 2024

@aditi-pandit @czentgr Please take a look.

@pdabre12 pdabre12 marked this pull request as ready for review August 5, 2024 23:57
@pdabre12 pdabre12 requested review from a team and elharo as code owners August 5, 2024 23:57
@pdabre12 pdabre12 requested a review from presto-oss August 5, 2024 23:57
steveburnett
steveburnett previously approved these changes Aug 6, 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, new local build, looks good. Thanks!

@tdcmeehan tdcmeehan self-assigned this Aug 6, 2024
@pdabre12
Copy link
Contributor Author

pdabre12 commented Aug 6, 2024 •
edited
Loading

@steveburnett Thank you for the review !

@pdabre12 pdabre12 mentioned this pull request Aug 6, 2024
Merged
aditi-pandit
aditi-pandit reviewed Aug 7, 2024
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

@pdabre12 : Thanks for this PR. Have one high level question.

presto-native-execution/presto_cpp/main/types/PrestoToVeloxExpr.cpp
{"presto.default.$operator$equal", "presto.default.eq"},
{"presto.default.$operator$greater_than", "presto.default.gt"},
{"presto.default.$operator$greater_than_or_equal", "presto.default.gte"},
{"presto.default.$operator$add",
Copy link
Contributor

@aditi-pandit aditi-pandit Aug 7, 2024

Choose a reason for hiding this comment

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

@pdabre12 : High level question about this... Should we be using prestoDefaultNamespacePrefix for the key function name in Velox as well ? Since we are registering functions with this prefix, then it should be used here as well.

Copy link
Contributor Author

@pdabre12 pdabre12 Aug 7, 2024

Choose a reason for hiding this comment

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

@aditi-pandit You are right. Thank you for catching this.
Updated my PR.

Copy link
Contributor Author

@pdabre12 pdabre12 Aug 15, 2024

Choose a reason for hiding this comment

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

@aditi-pandit Reverted the changes related to the operators and magic literal functions. These functions aren't reported by Prestissimo. Since, these don't get reported by Prestissimo they won't be present in the namespace specified by the presto.default-namespace property for eg. native.default namespace. As the operator types and magic literal functions are always passed down by the Java coordinator, it can be a fair assumption that they will have the presto.default namespace prefix.

@pdabre12 pdabre12 force-pushed the add-default-namespace-config-property branch 2 times, most recently from e27ee55 to dfa53df Compare August 7, 2024 21:11
aditi-pandit
aditi-pandit reviewed Aug 8, 2024
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

@pdabre12 : Would be great to add a unit test for this logic as well.

One possible test could be to retrieve all functions from Velox and check they have the default namespace.

presto-native-execution/presto_cpp/main/types/PrestoToVeloxExpr.cpp Outdated
{"presto.default.$operator$not_equal", "presto.default.neq"},
{"presto.default.$operator$subtract", "presto.default.minus"},
{"presto.default.$operator$subscript", "presto.default.subscript"},
{fmt::format("{}$operator$add", prestoDefaultNamespacePrefix),
Copy link
Contributor

@aditi-pandit aditi-pandit Aug 8, 2024

Choose a reason for hiding this comment

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

Write an inline function to add the defaultNamespacePrefix to both the operator name and Velox function name and use it for all entries below.

presto-native-execution/presto_cpp/main/types/PrestoToVeloxExpr.cpp Outdated
"presto.default.max_data_size_for_stats"},
{"presto.default.$internal$sum_data_size_for_stats",
"presto.default.sum_data_size_for_stats"},
{fmt::format(
Copy link
Contributor

@aditi-pandit aditi-pandit Aug 8, 2024

Choose a reason for hiding this comment

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

Write an inline function for adding the defaultNamespacePrefix to the function name and use it everywhere.

@pdabre12 pdabre12 force-pushed the add-default-namespace-config-property branch from dfa53df to 1379c6c Compare August 15, 2024 20:43
@pdabre12 pdabre12 force-pushed the add-default-namespace-config-property branch from 1379c6c to 48710fd Compare August 15, 2024 20:55
@pdabre12
Copy link
Contributor Author

pdabre12 commented Aug 15, 2024 •
edited
Loading

@aditi-pandit Thanks for the review.
Incorporated your suggestions, please have another look.

@pdabre12 pdabre12 closed this Feb 1, 2025
@pdabre12
Copy link
Contributor Author

pdabre12 commented Feb 1, 2025

Closing as completed via #23358.

@pdabre12 pdabre12 deleted the add-default-namespace-config-property branch February 1, 2025 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@czentgr czentgr czentgr left review comments

@aditi-pandit aditi-pandit aditi-pandit left review comments

@steveburnett steveburnett steveburnett left review comments

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

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

Assignees

@tdcmeehan tdcmeehan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[native] Enhancing namespace support for Prestissimo functions in native C++ clusters

5 participants

@pdabre12 @steveburnett @czentgr @aditi-pandit @tdcmeehan