Check type of filter expression at statement analysis #18460

takezoe · 2023-07-30T10:57:50Z

Description

Avoid INTERNAL ERROR (COMPILER_ERROR) when non boolean expression is given as FILTER predicate. With this fix, the error will be USER ERROR (TYPE_MISMATCH).

select count(*) filter(where 0) from system.runtime.queries;

io.trino.spi.TrinoException: Compiler failed. Possible reasons include: the query may have too many or too complex expressions, or the underlying tables may have too many columns
	at io.trino.sql.planner.LocalExecutionPlanner$Visitor.visitScanFilterAndProject(LocalExecutionPlanner.java:2110)
	at io.trino.sql.planner.LocalExecutionPlanner$Visitor.visitProject(LocalExecutionPlanner.java:1993)
	at io.trino.sql.planner.LocalExecutionPlanner$Visitor.visitProject(LocalExecutionPlanner.java:906)
	at io.trino.sql.planner.plan.ProjectNode.accept(ProjectNode.java:81)
	at io.trino.sql.planner.LocalExecutionPlanner$Visitor.visitAggregation(LocalExecutionPlanner.java:1928)
	at io.trino.sql.planner.LocalExecutionPlanner$Visitor.visitAggregation(LocalExecutionPlanner.java:906)
	at io.trino.sql.planner.plan.AggregationNode.accept(AggregationNode.java:221)
	at io.trino.sql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:628)
	at io.trino.sql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:533)
	at io.trino.execution.SqlTaskExecutionFactory.create(SqlTaskExecutionFactory.java:84)
	at io.trino.execution.SqlTask.tryCreateSqlTaskExecution(SqlTask.java:545)
	at io.trino.execution.SqlTask.updateTask(SqlTask.java:497)
	at io.trino.execution.SqlTaskManager.doUpdateTask(SqlTaskManager.java:532)
	at io.trino.execution.SqlTaskManager.lambda$updateTask$9(SqlTaskManager.java:476)
	at io.trino.$gen.Trino_dev____20230730_104711_2.call(Unknown Source)
	at io.trino.execution.SqlTaskManager.updateTask(SqlTaskManager.java:476)
	at io.trino.server.TaskResource.createOrUpdateTask(TaskResource.java:153)
	...
Caused by: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.RuntimeException: java.lang.VerifyError: Bad type on operand stack
Exception Details:
  Location:
    io/trino/$gen/CursorProcessor_20230730_104748_39.filter(Lio/trino/spi/connector/ConnectorSession;Lio/trino/spi/connector/RecordCursor;)Z @7: pop
  Reason:
    Type long_2nd (current frame, stack[1]) is not assignable to category1 type
  Current Frame:
    bci: @7
    flags: { }
    locals: { 'io/trino/$gen/CursorProcessor_20230730_104748_39', 'io/trino/spi/connector/ConnectorSession', 'io/trino/spi/connector/RecordCursor', integer }
    stack: { long, long_2nd }
  Bytecode:
    0000000: 033e 091d 9900 0557 03ac               
  Stackmap Table:
    full_frame(@9,{Object[#2],Object[#64],Object[#23],Integer},{Long})

	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2085)
	at com.google.common.cache.LocalCache.get(LocalCache.java:4011)
	at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4034)
	at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5010)
	at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5017)
	at com.google.common.cache.ForwardingLoadingCache.getUnchecked(ForwardingLoadingCache.java:54)
	at io.trino.sql.gen.ExpressionCompiler.compileCursorProcessor(ExpressionCompiler.java:85)
	at io.trino.sql.planner.LocalExecutionPlanner$Visitor.visitScanFilterAndProject(LocalExecutionPlanner.java:2075)
	... 83 more

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.

ebyhr · 2023-07-31T01:02:18Z

core/trino-main/src/test/java/io/trino/sql/analyzer/TestAnalyzer.java

@@ -7043,6 +7043,14 @@ public void testAlterTableAddRowField()
                .hasMessage("line 1:1: Adding fields with COMMENT is unsupported");
    }

+    @Test
+    public void testNNonBooleanFilterExpression()


I would move to the above testInvalidAggregationFilter method.

Suggested change

public void testNNonBooleanFilterExpression()

public void testNonBooleanFilterExpression()

Merged this case into testInvalidAggregationFilter.

martint · 2023-07-31T21:38:55Z

core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java

+                if (!type.equals(BOOLEAN)) {
+                    throw semanticException(TYPE_MISMATCH, expression, "Filter expression must evaluate to a boolean: actual type %s", type);
+                }


This breaks queries with ... FILTER (WHERE <expr>) where <expr> is known to evaluate to NULL at analysis time. E.g.,

SELECT count(*) FILTER (WHERE NULL) FROM (VALUES 1,2,3) t(x)

Take a look at analyzeWhere in SemanticAnalyzer for an example of how to handle this check. Also, please add a test for that case.

@martint NULL literal is not allowed in FILTER clause. FILTER (WHERE NULL) fails even without this PR.

trino/core/trino-main/src/main/java/io/trino/sql/planner/plan/FilterNode.java

Lines 46 to 48 in 67327de

// The condition doesn't guarantee that predicate is of type boolean, but was found to be a practical way to identify

// places where FilterNode was created without appropriate coercions.

checkArgument(!(predicate instanceof NullLiteral), "Predicate must be an expression of boolean type: %s", predicate);

FilterNode is an IR operation to represent the application of a filter, and is unrelated to the FILTER clause in an aggregation.

What’s not allowed is for the IR to contain a NullLiteral without an intervening cast that indicates what type it’s supposed to be, since that information is not included in the NullLiteral class.

It does work, BTW. What query did you try?

trino> SELECT count(*) FILTER (WHERE NULL) FROM (VALUES 1,2,3) t(x); _col0 ------- 0 (1 row)

is unrelated to the FILTER clause in an aggregation

Ah, I see. Thanks. I had only tested SELECT count(*) FILTER (WHERE NULL) FROM t1.

Updated the PR to allow UNKNOWN type.

martint · 2023-08-01T14:56:01Z

core/trino-main/src/test/java/io/trino/sql/analyzer/TestAnalyzer.java

@@ -4032,6 +4032,12 @@ public void testInValidJoinOnClause()
                .hasMessage("line 1:69: JOIN ON clause must evaluate to a boolean: actual type row(boolean, boolean)");
    }

+    @Test
+    public void testValidAggregationFilter()


This name is too vague. Change it to testNullAggregationFilter

cla-bot bot added the cla-signed label Jul 30, 2023

ebyhr approved these changes Jul 31, 2023

View reviewed changes

ebyhr requested review from martint and kasiafi July 31, 2023 01:33

takezoe force-pushed the check-filter-expression-type branch from 490776e to 7685f58 Compare July 31, 2023 16:49

martint requested changes Jul 31, 2023

View reviewed changes

takezoe force-pushed the check-filter-expression-type branch from 7685f58 to f843c04 Compare August 1, 2023 00:29

martint reviewed Aug 1, 2023

View reviewed changes

Check type of filter expression at statement analysis

1ee04ba

takezoe force-pushed the check-filter-expression-type branch from f843c04 to 1ee04ba Compare August 1, 2023 22:41

martint merged commit 70ee572 into trinodb:master Aug 2, 2023

github-actions bot added this to the 423 milestone Aug 2, 2023

colebow mentioned this pull request Aug 2, 2023

Add Trino 423 release notes #18496

Merged

takezoe mentioned this pull request Aug 5, 2023

Coerce boolean type for filter expression #18551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check type of filter expression at statement analysis #18460

Check type of filter expression at statement analysis #18460

takezoe commented Jul 30, 2023 •

edited by ebyhr

Loading

ebyhr Jul 31, 2023

takezoe Jul 31, 2023

martint Jul 31, 2023

takezoe Jul 31, 2023 •

edited

Loading

martint Aug 1, 2023

martint Aug 1, 2023

takezoe Aug 1, 2023 •

edited

Loading

takezoe Aug 1, 2023 •

edited

Loading

martint Aug 1, 2023

takezoe Aug 1, 2023

	public void testNNonBooleanFilterExpression()
	public void testNonBooleanFilterExpression()

	// The condition doesn't guarantee that predicate is of type boolean, but was found to be a practical way to identify
	// places where FilterNode was created without appropriate coercions.
	checkArgument(!(predicate instanceof NullLiteral), "Predicate must be an expression of boolean type: %s", predicate);

Check type of filter expression at statement analysis #18460

Check type of filter expression at statement analysis #18460

Conversation

takezoe commented Jul 30, 2023 • edited by ebyhr Loading

Description

Additional context and related issues

Release notes

ebyhr Jul 31, 2023

Choose a reason for hiding this comment

takezoe Jul 31, 2023

Choose a reason for hiding this comment

martint Jul 31, 2023

Choose a reason for hiding this comment

takezoe Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

martint Aug 1, 2023

Choose a reason for hiding this comment

martint Aug 1, 2023

Choose a reason for hiding this comment

takezoe Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

takezoe Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

martint Aug 1, 2023

Choose a reason for hiding this comment

takezoe Aug 1, 2023

Choose a reason for hiding this comment

takezoe commented Jul 30, 2023 •

edited by ebyhr

Loading

takezoe Jul 31, 2023 •

edited

Loading

takezoe Aug 1, 2023 •

edited

Loading

takezoe Aug 1, 2023 •

edited

Loading