Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201706090124.v591Og6Y002910@ip-10-146-233-104.ec2.internal>
Date: Fri, 9 Jun 2017 01:24:42 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: Lars Volker <lv@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Taras Bobrovytsky <tbobrovytsky@cloudera.com>,
        Jim Apple <jbapple-impala@apache.org>,
        Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3643/IMPALA-5344=3A_Fix_FE_tests_on_Java_8=0A?=
In-Reply-To: <gerrit.1496566826000.Iad8e1dccec3a51293a109c420bd2b88b9d1e0625@gerrit.cloudera.org>
References: <gerrit.1496566826000.Iad8e1dccec3a51293a109c420bd2b88b9d1e0625@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Fri, 09 Jun 2017 01:24:47 -0000

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................


Patch Set 5:

(6 comments)

I had a few high-level comments:
* A lot of this depends on order being preserved across multiple classes. If one class depends on another class returning things in a particular order, we should document that as part of the interface (either in a comment or via the types)
* In a lot of cases, if it's important that a particular data structure preserves insertion order, we should declare the member or return value as a LinkedHash* instead of just a Hash*. Otherwise it's all very mysterious.
* I think in many cases, instead of trying to preserve an arbitrary order across multiple processing steps, we should instead put the elements into a canonical order at the point where we iterate over them. It's hard to understand why things work otherwise.

http://gerrit.cloudera.org:8080/#/c/7073/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

Line 443:       Map<String, List<PrivilegeRequest>> tablePrivReqs = Maps.newLinkedHashMap();
Mention that the LinkedHashMap is used to preserve the order. I also wonder if TreeMap would be better - that way the order would be alphabetical rather than arbitrary.


http://gerrit.cloudera.org:8080/#/c/7073/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 203:     public final Map<ExprId, Expr> conjuncts = Maps.newLinkedHashMap();
The changes in here all feel a bit arbitrary. Do we understand why it's necessary to change all of these?


Line 1811:   private Map<EquivalenceClassId, List<SlotId>> getEquivClasses(List<TupleId> tids) {
I don't understand why the order should matter here. It looks like there is some interaction between this, DisjointSet, and  createEquivConjuncts() but it's not clear what the contract between the components is with regards to ordering.


http://gerrit.cloudera.org:8080/#/c/7073/5/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
File fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java:

It's really hard to understand why the ordering is necessary in most of these places or what the contracts are between the different data structures. Is there a way we can just enforce the ordering on output, rather than trying to maintain an arbitrary order the whole way through.


http://gerrit.cloudera.org:8080/#/c/7073/5/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java:

Line 187:         Maps.newLinkedHashMap(innerStmt.getTblProperties());
It was not obvious the me that getTblProperties() had any specific order. I had to look at the cup file to figure that out!


http://gerrit.cloudera.org:8080/#/c/7073/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java:

Line 437:   public Set<RuntimeFilter> getRuntimeFilters() {
I think it would be less brittle to just return a list of filters sorted by filterId here. That also will make it less likely that we will unintentionally change the result if we change the algorithms in here.


-- 
To view, visit http://gerrit.cloudera.org:8080/7073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes