Skip to content

Disallow aggregation function as UNNEST parameter #17988

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

Merged
kokosing merged 1 commit into from
Jun 27, 2023
+18 −6
Merged

Disallow aggregation function as UNNEST parameter #17988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions core/trino-main/src/main/java/io/trino/metadata/InternalFunctionBundle.java
View file
Open in desktop
Comment thread
radek-kondziolka marked this conversation as resolved.
Outdated
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,9 @@ public ScalarFunctionImplementation getScalarFunctionImplementation(

private SpecializedSqlScalarFunction specializeScalarFunction(FunctionId functionId, BoundSignature boundSignature, FunctionDependencies functionDependencies)
{
SqlScalarFunction function = (SqlScalarFunction) getSqlFunction(functionId);
return function.specialize(boundSignature, functionDependencies);
SqlFunction function = getSqlFunction(functionId);
checkArgument(function instanceof SqlScalarFunction, "%s is not a scalar function", function.getFunctionMetadata().getSignature());
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].

return ((SqlScalarFunction) function).specialize(boundSignature, functionDependencies);
}

@Override
Expand All @@ -156,8 +157,9 @@ public AggregationImplementation getAggregationImplementation(FunctionId functio

private AggregationImplementation specializedAggregation(FunctionId functionId, BoundSignature boundSignature, FunctionDependencies functionDependencies)
{
SqlAggregationFunction aggregationFunction = (SqlAggregationFunction) functions.get(functionId);
return aggregationFunction.specialize(boundSignature, functionDependencies);
SqlFunction function = getSqlFunction(functionId);
checkArgument(function instanceof SqlAggregationFunction, "%s is not an aggregation function", function.getFunctionMetadata().getSignature());
return ((SqlAggregationFunction) function).specialize(boundSignature, functionDependencies);
}

@Override
Expand All @@ -174,8 +176,9 @@ public WindowFunctionSupplier getWindowFunctionSupplier(FunctionId functionId, B

private WindowFunctionSupplier specializeWindow(FunctionId functionId, BoundSignature boundSignature, FunctionDependencies functionDependencies)
{
SqlWindowFunction function = (SqlWindowFunction) functions.get(functionId);
return function.specialize(boundSignature, functionDependencies);
SqlFunction function = functions.get(functionId);
checkArgument(function instanceof SqlWindowFunction, "%s is not a window function", function.getFunctionMetadata().getSignature());
return ((SqlWindowFunction) function).specialize(boundSignature, functionDependencies);
}

private SqlFunction getSqlFunction(FunctionId functionId)
Expand Down
1 change: 1 addition & 0 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1512,6 +1512,7 @@ protected Scope visitUnnest(Unnest node, Optional<Scope> scope)

ImmutableList.Builder<Field> outputFields = ImmutableList.builder();
for (Expression expression : node.getExpressions()) {
verifyNoAggregateWindowOrGroupingFunctions(session, metadata, expression, "UNNEST");
List<Field> expressionOutputs = new ArrayList<>();

ExpressionAnalysis expressionAnalysis = analyzeExpression(expression, createScope(scope));
Expand Down
8 changes: 8 additions & 0 deletions core/trino-main/src/test/java/io/trino/sql/analyzer/TestAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6681,6 +6681,14 @@ public void testJsonTable()
.hasMessage("line 1:15: JSON_TABLE is not yet supported");
}

@Test
public void testDisallowAggregationFunctionInUnnest()
{
assertFails("SELECT a FROM (VALUES (1), (2)) t(a), UNNEST(ARRAY[COUNT(t.a)])")
.hasErrorCode(EXPRESSION_NOT_SCALAR)
.hasMessage("line 1:46: UNNEST cannot contain aggregations, window functions or grouping operations: [COUNT(t.a)]");
}

@BeforeClass
public void setup()
{
Expand Down