Skip to content

Replace invalid Lookup.resolveGroup usage with resolve#14595

Merged
raunaqmorarka merged 3 commits intotrinodb:masterfrom
starburstdata:ls/048-resolve-group-fix
Oct 14, 2022
Merged

Replace invalid Lookup.resolveGroup usage with resolve#14595
raunaqmorarka merged 3 commits intotrinodb:masterfrom
starburstdata:ls/048-resolve-group-fix

Conversation

@lukasz-stec
Copy link
Copy Markdown
Member

@lukasz-stec lukasz-stec commented Oct 12, 2022

Description

Since Lookup.resolveGroup returns sub-plan
alternatives, the results cannot be joined
inside a single plan node.

Non-technical explanation

Release notes

(X ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Oct 12, 2022
@lukasz-stec lukasz-stec changed the title Replace invalid Lookup .resolveGroup usage with resolve Replace invalid Lookup.resolveGroup usage with resolve Oct 12, 2022
findepi
findepi reviewed Oct 13, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/cost/FilterProjectAggregationStatsRule.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Oct 13, 2022

Choose a reason for hiding this comment

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

Separate this from "Replace invalid resolveGroup usage with resolve" commit.

This place wasn't incorrect.
it was actually correct, and just working around the (premature) deprecation of resolveGroup, doing same logic as resolve, but with a local (non deprecated) helper method

Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Oct 13, 2022

Choose a reason for hiding this comment

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

Moved to "Deduplicate Lookup.resolve copies" commit

findepi
findepi reviewed Oct 13, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PruneDistinctAggregation.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Oct 13, 2022

Choose a reason for hiding this comment

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

Separate this from "Replace invalid resolveGroup usage with resolve" commit.

This wasn't incorrect. That was just overly verbose, i.e. using resolveGroup without real group capability

Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Oct 13, 2022

Choose a reason for hiding this comment

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

Moved to "Deduplicate Lookup.resolve copies" commit

@findepi
Copy link
Copy Markdown
Member

findepi commented Oct 13, 2022

thank you for your PR

@lukasz-stec lukasz-stec force-pushed the ls/048-resolve-group-fix branch from d5d3e3e to 4f033ec Compare October 13, 2022 12:36
lukasz-stec
lukasz-stec commented Oct 13, 2022
View reviewed changes
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

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

CA

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PruneDistinctAggregation.java Outdated
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Oct 13, 2022

Choose a reason for hiding this comment

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

Moved to "Deduplicate Lookup.resolve copies" commit

core/trino-main/src/main/java/io/trino/cost/FilterProjectAggregationStatsRule.java Outdated
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Oct 13, 2022

Choose a reason for hiding this comment

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

Moved to "Deduplicate Lookup.resolve copies" commit

@lukasz-stec lukasz-stec requested a review from findepi October 13, 2022 12:36
@findepi
Copy link
Copy Markdown
Member

findepi commented Oct 13, 2022

CI is sad, so i cancelled the oustanding jobs. ptal

@lukasz-stec lukasz-stec force-pushed the ls/048-resolve-group-fix branch 2 times, most recently from 5ab3b9a to fa9b431 Compare October 14, 2022 07:03
raunaqmorarka
raunaqmorarka approved these changes Oct 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka left a comment

Choose a reason for hiding this comment

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

lgtm % removal of "Update Trino 400 release notes" commit

@lukasz-stec
Replace invalid resolveGroup usage with resolve
1cf8af8 
Since Lookup.resolveGroup returns sub-plan
alternatives, the results cannot be joined
inside single plan node.
@lukasz-stec
Deduplicate Lookup.resolve copies
c9f88a7
@lukasz-stec
Use toImmutableList() instead of toList()
f7401d7
@lukasz-stec lukasz-stec force-pushed the ls/048-resolve-group-fix branch from fa9b431 to f7401d7 Compare October 14, 2022 08:22
@lukasz-stec
Copy link
Copy Markdown
Member Author

lukasz-stec commented Oct 14, 2022

CI failure of TestS3WrongRegionPicked.testS3WrongRegionSelection due to #14568.
Fixed already in the master

@raunaqmorarka raunaqmorarka merged commit 4cf2102 into trinodb:master Oct 14, 2022
@raunaqmorarka raunaqmorarka deleted the ls/048-resolve-group-fix branch October 14, 2022 12:06
@github-actions github-actions bot added this to the 401 milestone Oct 14, 2022
@colebow colebow mentioned this pull request Oct 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findepi findepi findepi approved these changes

@raunaqmorarka raunaqmorarka raunaqmorarka approved these changes

@martint martint Awaiting requested review from martint

Assignees

No one assigned

Labels

cla-signed

Milestone

401

Development

Successfully merging this pull request may close these issues.

3 participants

@lukasz-stec @findepi @raunaqmorarka