Skip to content

Disallow aggregation function as UNNEST parameter#17988

Merged
kokosing merged 1 commit intotrinodb:masterfrom
radek-kondziolka:rk/add_invariant
Jun 27, 2023
Merged

Disallow aggregation function as UNNEST parameter#17988
kokosing merged 1 commit intotrinodb:masterfrom
radek-kondziolka:rk/add_invariant

Conversation

@radek-kondziolka
Copy link
Copy Markdown
Contributor

@radek-kondziolka radek-kondziolka commented Jun 21, 2023 •
edited by kokosing
Loading

Description

Disallow aggregation function as UNNEST parameter and add similar check as here: ad637f7#diff-bd8fefc1e4638b7f65e377b75c16b46a2c541a24a64993860663507351917230R106

Release notes

General

  • Improve the error message when using aggregation function as input for UNNEST clause

@cla-bot cla-bot bot added the cla-signed label Jun 21, 2023
@radek-kondziolka radek-kondziolka force-pushed the rk/add_invariant branch 2 times, most recently from 8c197af to ad637f7 Compare June 21, 2023 11:55
kokosing
kokosing reviewed Jun 21, 2023
View reviewed changes
core/trino-main/src/main/java/io/trino/metadata/InternalFunctionBundle.java Outdated
Copy link
Copy Markdown
Member

@kokosing kokosing Jun 21, 2023

Choose a reason for hiding this comment

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

We need unit tests for that and this is an SQL error so we need a proper UX error.

Copy link
Copy Markdown
Contributor Author

@radek-kondziolka radek-kondziolka Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

On my eye it is not SQL error. It is kind of invariant that should be satisfied here. We have similar here:
ad637f7#diff-bd8fefc1e4638b7f65e377b75c16b46a2c541a24a64993860663507351917230R106
I can add test but I am not sure that we test invariants in other places. This invariant is to help detect such problem immediately in the future. But, the end user should not observe this (if there is no bug in code).

Copy link
Copy Markdown
Member

@electrum electrum Jun 21, 2023

Choose a reason for hiding this comment

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

This should already be handled during SQL analysis by io.trino.sql.analyzer.Analyzer#verifyNoAggregateWindowOrGroupingFunctions as the newly added test shows.

Copy link
Copy Markdown
Contributor Author

@radek-kondziolka radek-kondziolka Jun 22, 2023

Choose a reason for hiding this comment

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

Yes, this case is handled by StatementAnalyzer now. This is why I've put assertions, as the last ressort. [The first PR did not include verifyNoAggregateWindowOrGroupingFunctions so it was true then that it was exposed to the end user].

@radek-kondziolka radek-kondziolka changed the title Assert that specializeScalarFunction got scalar function Disallow aggregation function as UNNEST parameter Jun 21, 2023
@radek-kondziolka radek-kondziolka requested a review from kasiafi June 21, 2023 14:26
@radek-kondziolka radek-kondziolka mentioned this pull request Jun 21, 2023
Closed
@kokosing kokosing merged commit 430c964 into trinodb:master Jun 27, 2023
@github-actions github-actions bot added this to the 421 milestone Jun 27, 2023
@colebow colebow mentioned this pull request Jun 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@electrum electrum electrum left review comments

@kasiafi kasiafi kasiafi left review comments

@kokosing kokosing kokosing approved these changes

@sopel39 sopel39 Awaiting requested review from sopel39

Assignees

No one assigned

Labels

cla-signed

Milestone

421

Development

Successfully merging this pull request may close these issues.

4 participants

@radek-kondziolka @electrum @kokosing @kasiafi