impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.
Date Thu, 30 Nov 2017 00:27:09 GMT
Alex Behm has posted comments on this change. ( )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.

Patch Set 8:

File fe/src/main/java/org/apache/impala/analysis/
PS8, Line 326:    *    condition using the LHS constant. The rewrite handles group by,
Not clear what "handles" here means. It doesn't really do anything special to group by, limit,

Maybe instead say something like: The inline view ensures that the filter is logically evaluated
against the RHS result even if the RHS contains analytics/aggregation/limit. Adding the filter
directly to any clause within the RHS statement is generally incorrect.
PS8, Line 327:    *    analytic functions, limit, and uncorrelated subqueries. Correlated
Move last sentence into a TODO like this:

TODO: Correlated NOT IN subqueries require that column resolution be extended to handle ...
PS8, Line 330:    *    TODO: handle correlated NOT IN case
remove (see above comment)
PS8, Line 340:    *    So, if C is equal to any x_i, the expression is false. Similarly, if
Nice description. I'm wondering if we directly translate this insight into the WHERE condition
to make it easier to understand:


That way we can also shrink/remove the last paragraph in this comment block.
PS8, Line 355:     Preconditions.checkArgument(rhs.contains(Subquery.class));
not needed because L356-357 performs the same check
PS8, Line 398:     newSubquery.analyze(rhsQuery.getAnalyzer());
This seems wrong. It might happen to work but pretty sure this will eventually lead to trouble.
Each query block needs to get its own analyzer and the hierarchy of Analyzers needs to be

I'm thinking we should reset the rhsQuery and not analyze newSubquery here. The caller will
analyze the new ExistsPredicate with the Analyzer of the enclosing stmt (the correct analyzer).
PS8, Line 714:     if (isCorrelatedPredicate(subqueryStmt.getWhereClause(), subqueryTupleIds))
File fe/src/test/java/org/apache/impala/analysis/
PS8, Line 277:       // NOT IN subquery with a correlated non-equi predicate is ok if the
subquery only
[NOT] IN subquery
PS8, Line 292:         AnalyzesOk(String.format("select 1 from functional.allcomplextypes
t " +
What about queries without complex types and non-constant LHS in the IN predicate? Not sure
that case has coverage now.
PS8, Line 299:       AnalysisError(String.format("select * from functional.alltypestiny t
where id %s " +
These tests are pretty rough to follow, perhaps we should spent some time brainstorming how
we can clean up these tests (after this patch of course)
PS8, Line 437:       // Tests for constant on the left hand side of [NOT] IN.
Since we are doing a double rewrite here, it's interesting to test nested subqueries. Can
you add 1-2 tests for that?

Since you already did the work of separating these tests, it seems good to add them into a
new @Test, something like TestConstantLhsInSubqueries()
PS8, Line 476:  
You can omit this part if you like. Error message matching is prefix based.

You can use this to clean up other AnalysisError() calls. Imo it's not always useful to include
the whole failed stmt
PS8, Line 482:               "SELECT int_col FROM functional.alltypestiny b WHERE =
ORDER BY int_col ASC LIMIT 5");
long line, you can omit this line in the expected error message
PS8, Line 520:         + "(select * from functional.tinyinttable x where = x.int_col)");
please move "+" to previous line (easier to read and more consistent with existing style)
File testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test:
PS8, Line 2117: # IMPALA-1422: Constant on LHS of IN, uncorrelated subquery
Let's not add the JIRA number. We typically only do that for bugfixes, this is a new feature.
PS8, Line 2118: select * from functional.alltypessmall where 1 in (select int_col from functional.alltypestiny);
no need to trailing semicolon
PS8, Line 2140: |     predicates: 1 IS NULL OR CASE WHEN int_col IS NULL THEN 1 ELSE int_col
END = 1
Need to update this based on the new WHERE predicate
PS8, Line 2147: select * from functional.alltypessmall a where 1 in (select int_col from functional.alltypestiny
b where =;
long lines (here and elsewhere)
PS8, Line 2219: |  
PS8, Line 2281: ====
Add a case with nested subqueries to make sure the generated plan is sane
File testdata/workloads/functional-query/queries/QueryTest/subquery.test:
PS8, Line 839: ---- QUERY
Nice tests!

Move into a new file constant-lhs-in-subquery or something. This file is becoming pretty unwieldy.

We should probably split up even more sometime later.
PS8, Line 872: # Constant on LHS of [NOT] IN. Basics to test:
Move to the top?
PS8, Line 901: # LHS null, Predicate NOT IN, RHS NULL. Expect no results.
Comment does not match SQL
PS8, Line 1126: ====
Add 1-2 tests with nested subqueries.

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 8
Gerrit-Owner: Vuk Ercegovac <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-Comment-Date: Thu, 30 Nov 2017 00:27:09 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message