impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Date Fri, 16 Jun 2017 22:45:05 GMT
Lars Volker has posted comments on this change.

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


Patch Set 6:

(6 comments)

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:       // LinkedHashMap to preserve the order in which requests are inserted.
> Mention that the LinkedHashMap is used to preserve the order. I also wonder
Done. LinkedHashMap preserves the order in which elements are inserted and likely is closer
in performance characteristics to a HashMap than TreeMap would be. Let me know which one you
prefer, I don't have a strong preference.


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: 
> The changes in here all feel a bit arbitrary. Do we understand why it's nec
Some of these were necessary after debugging and realizing that the order of the them matters.
I ended up changing all of them for consistency. After revisiting them, I removed some of
them again.


Line 1811:    * to the given tuple ids. Only contains equivalence classes with more than one
member.
> I don't understand why the order should matter here. It looks like there is
On second inspection, this wasn't needed indeed.


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 the
When working on this file I figured from looking at the expected results, that we're mostly
dealing with graphs, and that the order in which children of nodes were listed changed when
using the non-order-preserving data structures. That's why I changed most of them.

Based on your comment, I changed the code to order graph nodes before using them.


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:     HashMap<String, String> properties =
> It was not obvious the me that getTblProperties() had any specific order. I
I added a comment. Do you think we should re-sort the elements here?


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 List<RuntimeFilter> getRuntimeFilters() {
> I think it would be less brittle to just return a list of filters sorted by
Done


-- 
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: 6
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

Mime
View raw message