impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <>
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:

File fe/src/main/java/org/apache/impala/analysis/

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.
File fe/src/main/java/org/apache/impala/analysis/

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
> I don't understand why the order should matter here. It looks like there is
On second inspection, this wasn't needed indeed.
File fe/src/main/java/org/apache/impala/analysis/

> 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.
File fe/src/main/java/org/apache/impala/analysis/

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?
File fe/src/main/java/org/apache/impala/planner/

Line 437:   public List<RuntimeFilter> getRuntimeFilters() {
> I think it would be less brittle to just return a list of filters sorted by

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message