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

Enable logical property propagation by default #22266

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Enable logical property propagation by default #22266

merged 1 commit into from
Apr 9, 2024

Conversation

ClarenceThreepwood
Copy link
Contributor

@ClarenceThreepwood ClarenceThreepwood commented Mar 20, 2024 •
edited
Loading

Fixes #21968. Reenable logical property propagation after fixing #22171

== RELEASE NOTES ==

General Changes
* Enable propagation of logical properties by default

@ClarenceThreepwood ClarenceThreepwood force-pushed the enablelogiclprop branch 4 times, most recently from e1fb1ad to 8d6918a Compare March 26, 2024 20:00
@ClarenceThreepwood ClarenceThreepwood marked this pull request as ready for review March 26, 2024 22:10
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 26, 2024 •
edited
Loading

Codenotify: Notifying subscribers in CODENOTIFY files for diff 4f91dee...54ffa28.

Notify File(s)
@steveburnett presto-docs/src/main/sphinx/admin/properties.rst
presto-docs/src/main/sphinx/optimizer.rst
presto-docs/src/main/sphinx/optimizer/logical-properties.rst

@jaystarshot
Copy link
Member

cc: @feilong-liu

steveburnett
steveburnett requested changes Mar 27, 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 documentation! A few nits and some rephrasing suggestions mostly to improve readability.

presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/optimizer.rst Show resolved Hide resolved
presto-docs/src/main/sphinx/optimizer/logical-properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/optimizer/logical-properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/optimizer/logical-properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/optimizer/logical-properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/optimizer/logical-properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/optimizer/logical-properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/optimizer/logical-properties.rst Outdated Show resolved Hide resolved
steveburnett
steveburnett previously approved these changes Mar 28, 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 updated branch, new local build, everything looks good. Thanks!

@tdcmeehan tdcmeehan self-assigned this Mar 29, 2024
@mlyublena
Copy link
Contributor

Can you also add description to the PR and/or commit message about the bug that got fixed (in addition to changing the default value of the flag)?

@ClarenceThreepwood ClarenceThreepwood force-pushed the enablelogiclprop branch 3 times, most recently from 5785588 to 58b34fd Compare April 5, 2024 23:14
tdcmeehan
tdcmeehan approved these changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

LGTM

@ClarenceThreepwood ClarenceThreepwood merged commit 9e0958a into prestodb:master Apr 9, 2024
57 checks passed
@ClarenceThreepwood ClarenceThreepwood deleted the enablelogiclprop branch April 9, 2024 16:45
@steveburnett steveburnett mentioned this pull request Apr 11, 2024
Merged
6 tasks
@wanglinsong wanglinsong mentioned this pull request May 1, 2024
Merged
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlyublena mlyublena mlyublena left review comments

@tdcmeehan tdcmeehan tdcmeehan approved these changes

@steveburnett steveburnett steveburnett left review comments

@jaystarshot jaystarshot Awaiting requested review from jaystarshot jaystarshot 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

@feilong-liu feilong-liu Awaiting requested review from feilong-liu feilong-liu is a code owner

Assignees

@tdcmeehan tdcmeehan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable logical property framework by default
5 participants
@ClarenceThreepwood @jaystarshot @mlyublena @steveburnett @tdcmeehan