You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Lars Volker (Code Review)" <ge...@cloudera.org> on 2017/06/04 09:00:27 UTC
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Lars Volker has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7073
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
This change fixes the frontend tests to make them run on Java 8, mostly
by replacing HashMap with LinkedHashMap and HashSet with LinkedHashSet.
To test this I ran the tests using both Java 7 and Java 8 and made sure
they passed.
Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
M testdata/workloads/functional-planner/queries/PlannerTest/order.test
M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-views.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
12 files changed, 318 insertions(+), 313 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/7073/1
--
To view, visit http://gerrit.cloudera.org:8080/7073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7073/2/testdata/workloads/functional-planner/queries/PlannerTest/order.test
File testdata/workloads/functional-planner/queries/PlannerTest/order.test:
Line 694: | pass-through-operands: all
> I suspect the new one is better, since it skips the extra materialization s
I'm confused why this changed at all. The number of pass through operators should be independent of the original order of the union branches (there's logic to reorder the branches, putting the passthrough ones first).
Smells like there's a bug somewhere. Should maybe get Taras to have a look, since he added the passthrough logic.
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#4).
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
This change fixes the frontend tests to make them run on Java 8, by
replacing HashMap with LinkedHashMap and HashSet with LinkedHashSet
where needed.
To test this I ran the frontend tests using both Oracle Java 7 and
Oracle Java 8 and made sure they passed. I also verified that the tests
pass with OpenJDK 7.
Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
M testdata/workloads/functional-planner/queries/PlannerTest/order.test
M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-views.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
15 files changed, 334 insertions(+), 326 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/7073/4
--
To view, visit http://gerrit.cloudera.org:8080/7073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 6:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/7073/6/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
PS6, Line 1485: HashMap
> LinkedHashMap?
Done
PS6, Line 1528: Map
> LinkedHashMap?
Done
PS6, Line 1695:
> LinkedHashMap?
Done
PS6, Line 1908: HashMap
> LinkedHashMap?
Done
http://gerrit.cloudera.org:8080/#/c/7073/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
PS6, Line 444: Map
> LinkedHashMap? I don't feel as strongly about this one since it doesn't cro
Done
http://gerrit.cloudera.org:8080/#/c/7073/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
Line 179: // LinkeHashSet where applicable to preserve the iteration order and make the class
> typo
Done
http://gerrit.cloudera.org:8080/#/c/7073/6/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
File fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java:
PS6, Line 171: getSources
> getOrderedSources()?
Done
PS6, Line 176: getTargets
> Maybe getOrderedTargets()?
Done
PS6, Line 194: Map
> LinkedHashMap?
Done
Line 342: for (String target: Sets.newTreeSet(targets)) {
> It might be more efficient to use Guava's ImmutableSortedSet here and above
Done. Should we change this in other places in the file that we haven't touched so far?
http://gerrit.cloudera.org:8080/#/c/7073/6/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java:
PS6, Line 187: HashMap
> LinkedHashMap?
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7073/7/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
File fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java:
Line 173: return ImmutableSortedSet.copyOf(sources_);
> Fair enough, thanks for clarifying. As far as I can tell the fact that Immu
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 2:
(6 comments)
Thank you for the review. Please see my inline comments and PS4.
http://gerrit.cloudera.org:8080/#/c/7073/2//COMMIT_MSG
Commit Message:
PS2, Line 10: replacing
> Did you use a script to do a global replace in fe or did you pick-and-choos
I picked the ones in files related to tests that were failing, and replaced those that occurred in loops or otherwise looked suspicious.
PS2, Line 10: LinkedHashMap
> I assume LinkedHashThing takes more memory than HashThing. Is this somethin
Yes, LinkedHashThing has two references for the linked list, so it takes a bit more space. In return, it provides faster iteration. Here are the sources of the Node element for HashMap and LinkedHashMap:
http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/HashMap.java#l278
http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/LinkedHashMap.java#l192
I couldn't find benchmarks on the web, let me know if you think we should do one.
PS2, Line 13: a
> long line
Done
PS2, Line 13: Java 7 and Java 8
> OpenJDK or Oracle or both?
I ran tests for both, so I clarified it in the commit message.
http://gerrit.cloudera.org:8080/#/c/7073/2/testdata/workloads/functional-planner/queries/PlannerTest/order.test
File testdata/workloads/functional-planner/queries/PlannerTest/order.test:
Line 694: | pass-through-operands: all
> Are these plans functionally the same? If not, which one is better?
I suspect the new one is better, since it skips the extra materialization step. However, Taras may want to have a look at this since having less plans with non-all pass-through-operands may reduce coverage.
http://gerrit.cloudera.org:8080/#/c/7073/2/testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
File testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test:
Line 2411: predicates on o: !empty(o.o_lineitems), o_orderstatus = 'F'
> Does this difference show up also in the execution? If so, should we pick o
My assumption was that we only do cost based ordering for join predicates and that normal conjuncts are evaluated on the materialized tuples as a whole.
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 6:
Maybe the associated JIRA is a better place to have this discussion, but I am trying to understand the purpose of this patch. Why do we need FE tests to run against Java 8? Java 8 will be officially supported in C6 and until then we can't use any of the Java 8 features. Once we do switch to Java 8, can't we simply change the test results? There is only one case where order really matters (authorization).
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#8).
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
This change fixes the frontend tests to make them run on Java 8, by
replacing HashMap with LinkedHashMap and HashSet with LinkedHashSet
where needed.
To test this I ran the frontend tests using both Oracle Java 7 and
Oracle Java 8 and made sure they passed. I also verified that the tests
pass with OpenJDK 7.
Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterId.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
M testdata/workloads/functional-planner/queries/PlannerTest/order.test
M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
14 files changed, 330 insertions(+), 278 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/7073/8
--
To view, visit http://gerrit.cloudera.org:8080/7073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7073/2/testdata/workloads/functional-planner/queries/PlannerTest/order.test
File testdata/workloads/functional-planner/queries/PlannerTest/order.test:
Line 694: | pass-through-operands: all
> I'm confused why this changed at all. The number of pass through operators
I debugged this to find out why this changes. The child nodes need to pass a filter whether they can be passed through or not. One of the criteria is that all slot descriptors need to be the same, and before my change, some slots in the sort node had different offsets. I checked SortInfo.java and found a loop over the source slots using a HashSet (Line 193):
Set<SlotRef> sourceSlots = Sets.newHashSet();
This means the sort node would reorder slots, thus preventing the pass through. After changing this to a LinkedHashSet, the insertion order would be preserved, thus the slot offsets wouldn't change, and the check for pass-through-ability passed.
http://gerrit.cloudera.org:8080/#/c/7073/2/testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
File testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test:
Line 2411: predicates on o: !empty(o.o_lineitems), o_orderstatus = 'F'
> I think I don't quite understand - even if conjuncts are evaluated on mater
You're right, evaluation is aborted early if one fails (exec-node.cc:449). The order here is the order in which conjuncts are added to the analyzer and changes because GlobalState.conjuncts turned into a LinkedHashMap. Now it is somewhat more deterministic than before, but I agree that this can change performance for the better or worse.
As discussed offline I'll kick of a private perf run. Thank you for the suggestion.
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 6:
Lars can obviously chime in but I'll give my rationale for the +1.
My understanding is we will stay at Java language compatibility level 7 but remove the dependency of Impala tests on implementation-defined behaviour of Sun JDK 7. I think we should consider that dependence on implementation-defined behaviour a bug. There are no guarantees about iteration order in the Java spec and it could change at any point, e.g. in a JDK update or even between different instances of HashMaps.
OpenJDK 7 is near EOL and Sun JDK 7 is EOL so if we don't fix this we'll soon be in a situation where our tests only pass on obsolete JDKs.
This inconveniences me personally since I'm using OpenJDK 8 and in general it's unreasonable to expect contributors to develop on obsolete JDKs.
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7073/2/fe/src/main/java/org/apache/impala/analysis/SortInfo.java
File fe/src/main/java/org/apache/impala/analysis/SortInfo.java:
Line 193: Set<SlotRef> sourceSlots = Sets.newLinkedHashSet();
> We should add a comment that points out that using LinkedHashSets prevents
Done
http://gerrit.cloudera.org:8080/#/c/7073/4/testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
File testdata/workloads/functional-planner/queries/PlannerTest/lineage.test:
Line 35: "vertexId":"functional.alltypes.tinyint_col"
> Do we know why the order changes in this file?
Various places in ColumnLineageGraph.java iterated over HashSets. I changed those to LinkedHashSets to make the iteration order deterministic and consistent between Java 7 and Java 8, which changed the order 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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7073/2//COMMIT_MSG
Commit Message:
PS2, Line 10: LinkedHashMap
> Yes, LinkedHashThing has two references for the linked list, so it takes a
I'm more concerned about the space than the speed. Maybe Dimitris has some thoughts? It might not move the needle much in the context of other recent memory-saving changes.
http://gerrit.cloudera.org:8080/#/c/7073/2/testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
File testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test:
Line 2411: predicates on o: !empty(o.o_lineitems), o_orderstatus = 'F'
> My assumption was that we only do cost based ordering for join predicates a
I think I don't quite understand - even if conjuncts are evaluated on materialized tuples, order could matter, yes?
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7073/7/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
File fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java:
Line 173: return ImmutableSortedSet.copyOf(sources_);
> (the array is also ok, we don't need to churn the review necessarily, but w
Fair enough, thanks for clarifying. As far as I can tell the fact that ImmutableSortedSet it is a sorted array is implementation-defined behavior, and not part of its documented guarantees. Let's switch it back to ImmutableSortedSet.
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#9).
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
This change fixes the frontend tests to make them run on Java 8, by
replacing HashMap with LinkedHashMap and HashSet with LinkedHashSet
where needed.
To test this I ran the frontend tests using both Oracle Java 7 and
Oracle Java 8 and made sure they passed. I also verified that the tests
pass with OpenJDK 7.
Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterId.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
M testdata/workloads/functional-planner/queries/PlannerTest/order.test
M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
14 files changed, 321 insertions(+), 278 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/7073/9
--
To view, visit http://gerrit.cloudera.org:8080/7073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#5).
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
This change fixes the frontend tests to make them run on Java 8, by
replacing HashMap with LinkedHashMap and HashSet with LinkedHashSet
where needed.
To test this I ran the frontend tests using both Oracle Java 7 and
Oracle Java 8 and made sure they passed. I also verified that the tests
pass with OpenJDK 7.
Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
M testdata/workloads/functional-planner/queries/PlannerTest/order.test
M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-views.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
15 files changed, 336 insertions(+), 327 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/7073/5
--
To view, visit http://gerrit.cloudera.org:8080/7073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 7:
(3 comments)
Let's move forward with this change.
http://gerrit.cloudera.org:8080/#/c/7073/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
PS7, Line 206: p
Why is this change needed?
http://gerrit.cloudera.org:8080/#/c/7073/7/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
File fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java:
Line 123: this.label_.equals(vertex.label_);
Remove this to make compareTo() consistent with equals().
Line 173: return ImmutableSortedSet.copyOf(sources_);
Why not return an ordered list? Should be smaller, faster, and generate fewer useless intermediate objects.
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#6).
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
This change fixes the frontend tests to make them run on Java 8, by
replacing HashMap with LinkedHashMap and HashSet with LinkedHashSet
where needed.
To test this I ran the frontend tests using both Oracle Java 7 and
Oracle Java 8 and made sure they passed. I also verified that the tests
pass with OpenJDK 7.
Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterId.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
M testdata/workloads/functional-planner/queries/PlannerTest/order.test
M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
14 files changed, 310 insertions(+), 274 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/7073/6
--
To view, visit http://gerrit.cloudera.org:8080/7073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 2:
Dimitris, also there are changes the behavior of the union node. Pass through is enabled more often now (which is a good thing). I think it would be hard to make planner tests agnostic to that.
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 9: Code-Review+2
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 5:
Many of these changes are essentially needed just for generating consistent tests results when we switch between different java versions. I wonder if this the right approach. We're essentially "forcing" the use of a data structure with different performance/memory tradeoffs than we had before. It may not have significant impact on performance/memory usage, but creates confusion. For example, I believe we may end up overusing linked* just to ensure results stability. Wouldn't it be better is we made the test results verification agnostic to order? Just some thoughts...
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 6:
> Maybe the associated JIRA is a better place to have this
> discussion, but I am trying to understand the purpose of this
> patch. Why do we need FE tests to run against Java 8? Java 8 will
> be officially supported in C6 and until then we can't use any of
> the Java 8 features. Once we do switch to Java 8, can't we simply
> change the test results? There is only one case where order really
> matters (authorization).
C6 is a Cloudera matter, not one for Apache Impala.
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7073/7/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
File fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java:
Line 173: return ImmutableSortedSet.copyOf(sources_);
> ImmutableSortedSet *is* an ordered array.
(the array is also ok, we don't need to churn the review necessarily, but wanted to point out my motivation for suggesting ImmutableSortedSet in the first place)
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7073/4/testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
File testdata/workloads/functional-planner/queries/PlannerTest/lineage.test:
Line 35: "vertexId":"functional.alltypes.tinyint_col"
Do we know why the order changes in this file?
http://gerrit.cloudera.org:8080/#/c/7073/2/testdata/workloads/functional-planner/queries/PlannerTest/order.test
File testdata/workloads/functional-planner/queries/PlannerTest/order.test:
Line 694: | pass-through-operands: all
> I debugged this to find out why this changes. The child nodes need to pass
I don't think there's a bug, Tim. Before this patch, passthrough was not enabled for some cases where it could be, (because the tuples are not identical even though they could be). It looks like this patch fixes the problem, this is great! Passthrough will be enabled in more cases now, making Impala faster.
Also, I don't think that we lose that much coverage. We have tests in for both passthrough and non-passthrough cases in union.test. In general, we expect the union node to be in passthrough mode most of the time.
Line 694: | pass-through-operands: all
> Oh, cool, the reordering of the slots in the sort node seems unintentional.
Yes, it used to be unintentional, which looks like it's going to be fixed now.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 10: Verified+1
--
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 9:
Thank you for the review. I will rebase the change and make sure one more time that tests pass on both versions of Java.
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 5:
> Many of these changes are essentially needed just for generating
> consistent tests results when we switch between different java
> versions. I wonder if this the right approach. We're essentially
> "forcing" the use of a data structure with different
> performance/memory tradeoffs than we had before. It may not have
> significant impact on performance/memory usage, but creates
> confusion. For example, I believe we may end up overusing linked*
> just to ensure results stability. Wouldn't it be better is we made
> the test results verification agnostic to order? Just some
> thoughts...
When the order of some of these things change, the query execution can change which can affect performance (though hopefully not correctness). I'd imagine we would want the order to remain stable so as not to have performance regressions.
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 10:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/796/
--
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7073/2//COMMIT_MSG
Commit Message:
PS2, Line 10: replacing
Did you use a script to do a global replace in fe or did you pick-and-choose which ones to change?
PS2, Line 10: LinkedHashMap
I assume LinkedHashThing takes more memory than HashThing. Is this something w should be concerned about?
PS2, Line 13: a
long line
PS2, Line 13: Java 7 and Java 8
OpenJDK or Oracle or both?
http://gerrit.cloudera.org:8080/#/c/7073/2/testdata/workloads/functional-planner/queries/PlannerTest/order.test
File testdata/workloads/functional-planner/queries/PlannerTest/order.test:
Line 694: | pass-through-operands: all
Are these plans functionally the same? If not, which one is better?
http://gerrit.cloudera.org:8080/#/c/7073/2/testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
File testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test:
Line 2411: predicates on o: !empty(o.o_lineitems), o_orderstatus = 'F'
Does this difference show up also in the execution? If so, should we pick one of these predicates to go first based on selectivity?
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#3).
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
This change fixes the frontend tests to make them run on Java 8, by
replacing HashMap with LinkedHashMap and HashSet with LinkedHashSet
where needed.
To test this I ran the frontend tests using both Java 7 and Java 8 and
made sure they passed. I also verified that the tests pass with OpenJDK
7.
Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
M testdata/workloads/functional-planner/queries/PlannerTest/order.test
M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-views.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
15 files changed, 334 insertions(+), 326 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/7073/3
--
To view, visit http://gerrit.cloudera.org:8080/7073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 6:
(12 comments)
Looks good. Just have minor comments then I'm ready to +1. I think Alex or Dimitris should still have a pass over and confirm it makes sense ot them.
http://gerrit.cloudera.org:8080/#/c/7073/6/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
PS6, Line 1485: HashMap
LinkedHashMap?
PS6, Line 1528: Map
LinkedHashMap?
PS6, Line 1695:
LinkedHashMap?
PS6, Line 1908: HashMap
LinkedHashMap?
http://gerrit.cloudera.org:8080/#/c/7073/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
PS6, Line 444: Map
LinkedHashMap? I don't feel as strongly about this one since it doesn't cross module boundaries.
http://gerrit.cloudera.org:8080/#/c/7073/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
Line 179: // LinkeHashSet where applicable to preserve the iteration order and make the class
typo
http://gerrit.cloudera.org:8080/#/c/7073/6/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
File fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java:
PS6, Line 171: getSources
getOrderedSources()?
PS6, Line 176: getTargets
Maybe getOrderedTargets()?
PS6, Line 194: Map
LinkedHashMap?
Line 342: for (String target: Sets.newTreeSet(targets)) {
It might be more efficient to use Guava's ImmutableSortedSet here and above. It gives you back a Set<> backed by a sorted array. I don't feel that strongly but it seems like it would be easy enough.
http://gerrit.cloudera.org:8080/#/c/7073/6/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java:
PS6, Line 187: HashMap
LinkedHashMap?
PS6, Line 211: HashMap
LinkedHashMap?
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
This change fixes the frontend tests to make them run on Java 8, by
replacing HashMap with LinkedHashMap and HashSet with LinkedHashSet
where needed.
To test this I ran the frontend tests using both Oracle Java 7 and
Oracle Java 8 and made sure they passed. I also verified that the tests
pass with OpenJDK 7.
Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Reviewed-on: http://gerrit.cloudera.org:8080/7073
Reviewed-by: Lars Volker <lv...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterId.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
M testdata/workloads/functional-planner/queries/PlannerTest/order.test
M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
14 files changed, 321 insertions(+), 278 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Lars Volker: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7073/7/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
File fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java:
Line 173: List<Vertex> result = Lists.newArrayList(sources_);
> Done
ImmutableSortedSet *is* an ordered array.
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#7).
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
This change fixes the frontend tests to make them run on Java 8, by
replacing HashMap with LinkedHashMap and HashSet with LinkedHashSet
where needed.
To test this I ran the frontend tests using both Oracle Java 7 and
Oracle Java 8 and made sure they passed. I also verified that the tests
pass with OpenJDK 7.
Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterId.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
M testdata/workloads/functional-planner/queries/PlannerTest/order.test
M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
14 files changed, 320 insertions(+), 276 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/7073/7
--
To view, visit http://gerrit.cloudera.org:8080/7073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 10: Code-Review+2
Rebased, carrying Alex's +2.
--
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7073/7/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
File fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java:
Line 173: return ImmutableSortedSet.copyOf(sources_);
> ImmutableSortedSet *is* an ordered array.
Good point. I assumed there'd be some additional index structures, but that doesn't seem to be the case. The only thing we seem to save is an additional pass over the sorted array for deduplication, which in our case isn't necessary.
@Alex, should I change it back? I'm good with either.
https://github.com/google/guava/blob/master/guava/src/com/google/common/collect/ImmutableSortedSet.java#L387
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#2).
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
This change fixes the frontend tests to make them run on Java 8, by
replacing HashMap with LinkedHashMap and HashSet with LinkedHashSet
where needed.
To test this I ran the frontend tests using both Java 7 and Java 8 and made sure
they passed.
Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
M testdata/workloads/functional-planner/queries/PlannerTest/order.test
M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-views.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
15 files changed, 334 insertions(+), 326 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/7073/2
--
To view, visit http://gerrit.cloudera.org:8080/7073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad8e1dccec3a51293a109c420bd2b88b9d1e0625
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 6:
> Lars can obviously chime in but I'll give my rationale for the +1.
>
> My understanding is we will stay at Java language compatibility
> level 7 but remove the dependency of Impala tests on
> implementation-defined behaviour of Sun JDK 7. I think we should
> consider that dependence on implementation-defined behaviour a bug.
> There are no guarantees about iteration order in the Java spec and
> it could change at any point, e.g. in a JDK update or even between
> different instances of HashMaps.
>
> OpenJDK 7 is near EOL and Sun JDK 7 is EOL so if we don't fix this
> we'll soon be in a situation where our tests only pass on obsolete
> JDKs.
>
> This inconveniences me personally since I'm using OpenJDK 8 and in
> general it's unreasonable to expect contributors to develop on
> obsolete JDKs.
I agree with Tim: With Java 7 being at or close to EOL it would be nice to support running the tests on Java 8. This will also make setting up new development environments easier and fix out impala-setup chef scripts.
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 7:
(3 comments)
Thank you for the review. Please see my comments and PS8.
http://gerrit.cloudera.org:8080/#/c/7073/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
PS7, Line 206: p
> Why is this change needed?
Without it, the predicate order varies between different Java versions in the explain strings. On example where this matters is HdfsScanNode.java:890.
http://gerrit.cloudera.org:8080/#/c/7073/7/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
File fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java:
Line 123: this.label_.equals(vertex.label_);
> Remove this to make compareTo() consistent with equals().
Done
Line 173: return ImmutableSortedSet.copyOf(sources_);
> Why not return an ordered list? Should be smaller, faster, and generate few
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7073/2/fe/src/main/java/org/apache/impala/analysis/SortInfo.java
File fe/src/main/java/org/apache/impala/analysis/SortInfo.java:
Line 193: Set<SlotRef> sourceSlots = Sets.newLinkedHashSet();
We should add a comment that points out that using LinkedHashSets prevents the slots getting reordered unnecessarily.
http://gerrit.cloudera.org:8080/#/c/7073/2/testdata/workloads/functional-planner/queries/PlannerTest/order.test
File testdata/workloads/functional-planner/queries/PlannerTest/order.test:
Line 694: | pass-through-operands: all
> I debugged this to find out why this changes. The child nodes need to pass
Oh, cool, the reordering of the slots in the sort node seems unintentional.
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3643/IMPALA-5344: Fix FE tests on Java 8
......................................................................
Patch Set 5:
I'm reluctant to hold up the fix because it's important, but I think if this goes in as-is it could cause a lot of avoidable confusion in future.
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No