impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vuk Ercegovac (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.
Date Wed, 25 Oct 2017 21:38:35 GMT
Vuk Ercegovac has posted comments on this change. ( )

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

Patch Set 5:


missed the planner tests last time; they are included in the next patch.
File fe/src/main/java/org/apache/impala/analysis/
PS5, Line 228: 	  if (newConjunct instanceof BoolLiteral) {
             : 	      smap.put(conjunct, newConjunct);
             : 	      continue;
             : 	  }
> nit: remove tabs. Also, I am not sure I understand what is happening here.
removed... I had an earlier attempt that tried to collapse the case where lhs is null.
PS5, Line 308: correlated aggregates
> what is a correlated aggregate?
PS5, Line 323: c
> use capital C just to be consistent with L321
PS5, Line 329: 10 IS NULL OR
> Since we know what the constant literal is, can't we avoid the is null chec
I tried this but saw that for example, (1 + null) did not get simplified to null when this
rewrite was invoked. Even if a case like this is simplified, will we guarantee all such simplifications?
(we should, but I cannot depend on that at the moment).
PS5, Line 355: or 10 IS NOT NULL
> Same comment as above.
comment had an error. fixed.
PS5, Line 360: .
> nit: remove '.'
PS5, Line 366: 
> nit: remove empty lines (L366, L369 and 371)
PS5, Line 428: newSubquery.reset();
             :       newSubquery.analyze(rhsQuery.getAnalyzer());
> Why do you need to go through reset/analyze? Same question for L459-460.
the select was cloned, so is in an analyzed state. added a comment. let me know if this should
be handled in some other way-- I can assume that caller code will cover this case, and currently
it will-- but I opted to be explicit about it here.

The case for L459 is a new select, so I've removed that reset.

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: 5
Gerrit-Owner: Vuk Ercegovac <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-Comment-Date: Wed, 25 Oct 2017 21:38:35 +0000
Gerrit-HasComments: Yes

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