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.

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

Fix output symbols of PushFilterThroughBoolOrAggregation optimizer #22716

Merged
merged 1 commit into from
Jul 20, 2024

Conversation

jinyangli34
Copy link
Contributor

@jinyangli34 jinyangli34 commented Jul 17, 2024

Description

PushFilterThroughBoolOrAggregation optimizer may return more output symbol than original plan.
and cause error

java.lang.IllegalArgumentException: io.trino.sql.planner.iterative.rule.PushFilterThroughBoolOrAggregation$PushFilterThroughBoolOrAggregationWithoutProject: transformed expression doesn't produce same outputs: [project::[varchar], bool_or::[boolean]] vs [project::[varchar], is_mocked::[boolean], bool_or::[boolean]]
	at com.google.common.base.Preconditions.checkArgument(Preconditions.java:466)
	at io.trino.sql.planner.iterative.Memo.replace(Memo.java:117)

Fixing this issue by using output symbols from aggregation node instead of its source.

Additional context and related issues

example query to reproduce:

SELECT *
FROM
  (SELECT project, BOOL_OR(is_mocked) AS has_mocking
   FROM schema.table
   GROUP BY project) AS virtual_table
WHERE has_mocking = true

or see new unit test.

Release notes

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

# General
* Fix planning failure for queries with filter on an aggregation. ({issue}`22716`)

@cla-bot cla-bot bot added the cla-signed label Jul 17, 2024
@jinyangli34 jinyangli34 requested a review from Dith3r July 17, 2024 22:36
ProjectNode newProjectNode = new ProjectNode(
context.getIdAllocator().getNextId(),
newAggregationNode,
Assignments.builder()
.putIdentities(source.getOutputSymbols())
.putIdentities(otherSymbols)
Copy link
Member

@Dith3r Dith3r Jul 18, 2024

Choose a reason for hiding this comment

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

Use newAggregationNode.getOutputSymbols() and filtering is not required

@Dith3r Dith3r self-requested a review July 18, 2024 06:50
Copy link
Member

@Dith3r Dith3r left a comment

Choose a reason for hiding this comment

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

Please use newAggregationNode instead.

@jinyangli34 jinyangli34 force-pushed the jinyang-fix_bool_or_agg branch from b2296b3 to 8cc6691 Compare July 18, 2024 18:06
@jinyangli34
Copy link
Contributor Author

Please use newAggregationNode instead.

@Dith3r thanks, updated.

@Dith3r Dith3r requested a review from raunaqmorarka July 18, 2024 18:47
Copy link
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.

Please add the failing query as a test in io.trino.sql.query.TestCorrelatedAggregation#testBoolOrAggregation
LGTM otherwise

@raunaqmorarka raunaqmorarka requested review from martint and sopel39 July 19, 2024 06:01
@raunaqmorarka raunaqmorarka added the bug Something isn't working label Jul 19, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@jinyangli34 jinyangli34 force-pushed the jinyang-fix_bool_or_agg branch from 8cc6691 to 4b0582b Compare July 19, 2024 21:45
@jinyangli34
Copy link
Contributor Author

Please add the failing query as a test in io.trino.sql.query.TestCorrelatedAggregation#testBoolOrAggregation LGTM otherwise

Added test case and verified failure w/o fix vs pass w the fix.

@raunaqmorarka raunaqmorarka merged commit 4b58e8e into trinodb:master Jul 20, 2024
96 checks passed
@github-actions github-actions bot added this to the 453 milestone Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla-signed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants