[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking

2016-10-15 Thread Jim Apple (Code Review)
Jim Apple has posted comments on this change.

Change subject: IMPALA-4123: Fast bit unpacking
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4494/11/be/src/util/bit-packing.inline.h
File be/src/util/bit-packing.inline.h:

Line 57:   constexpr int BATCH_SIZE = 32;
After some reading, I think you can and should throw a static on these 
constexprs.

For instance, 
http://stackoverflow.com/questions/26152096/when-and-why-would-you-use-static-with-constexpr


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

2016-10-15 Thread Alex Behm (Code Review)
Alex Behm has uploaded a new patch set (#2).

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..

IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

AnayticExpr.analyze() replaces the original FIRST/LAST_VALUE
function with a FIRST/LAST_VALUE_IGNORE_NULLS function if
the IGNORE NULLS clause is specified.

The bug was that several places in AnalyticExpr.analyze() assumed
and asserted that only the original FIRST/LAST_VALUE function
could be encountered during analysis. However, with subquery
rewriting the IGNORE NULLS version of the function may also be
seen because the whole statement is re-analyzed after rewriting.

The fix is to also accept the IGNORE NULLS version of the function.

Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 22 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/4732/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 


[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS when subquery rewriting.

2016-10-15 Thread Alex Behm (Code Review)
Alex Behm has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4732

Change subject: IMPALA-4301: Fix IGNORE NULLS when subquery rewriting.
..

IMPALA-4301: Fix IGNORE NULLS when subquery rewriting.

AnayticExpr.analyze() replaces the original FIRST/LAST_VALUE
function with a FIRST/LAST_VALUE_IGNORE_NULLS function if
the IGNORE NULLS clause is specified.

The bug was that several places in AnalyticExpr.analyze() assumed
and asserted that only the original FIRST/LAST_VALUE function
could be encountered during analysis. However, with subquery
rewriting the IGNORE NULLS version of the function may also be
seen because the whole statement is re-analyzed after rewriting.

The fix is to also accept the IGNORE NULLS version of the function.

Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 22 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/4732/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 


[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

2016-10-15 Thread Alex Behm (Code Review)
Alex Behm has uploaded a new patch set (#3).

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..

IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

AnayticExpr.analyze() replaces the original FIRST/LAST_VALUE
function with a FIRST/LAST_VALUE_IGNORE_NULLS function if
the IGNORE NULLS clause is specified.

The bug was that several places in AnalyticExpr.analyze() assumed
and asserted that only the original FIRST/LAST_VALUE function
could be encountered during analysis. However, with subquery
rewriting the IGNORE NULLS version of the function may also be
seen because the whole statement is re-analyzed after rewriting.

The fix is to also accept the IGNORE NULLS version of the function.

Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 23 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/4732/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 


[Impala-ASF-CR] IMPALA-2905: Handle coordinator fragment lifecycle like all others

2016-10-15 Thread Henry Robinson (Code Review)
Hello Marcel Kornacker, Internal Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4402

to look at the new patch set (#20).

Change subject: IMPALA-2905: Handle coordinator fragment lifecycle like all 
others
..

IMPALA-2905: Handle coordinator fragment lifecycle like all others

The plan-root fragment instance that runs on the coordinator should be
handled like all others: started via RPC and run asynchronously. Without
this, the fragment requires special-case code throughout the
coordinator, and does not show up in system metrics etc.

This patch adds a new sink type, PlanRootSink, to the root fragment
instance so that the coordinator can pull row batches that are pushed by
the root instance. The coordinator signals completion to the fragment
instance via closing the consumer side of the sink, whereupon the
instance is free to complete.

Since the root instance now runs asynchronously wrt to the coordinator,
we add several coordination methods to allow the coordinator to wait for
a point in the instance's execution to be hit - e.g. to wait until the
instance has been opened.

Done in this patch:

* Add PlanRootSink
* Add coordination to PFE to allow coordinator to observe lifecycle
* Make FragmentMgr a singleton
* Removed dead code from Coordinator::Wait() and elsewhere.
* Moved result output exprs out of QES and into PlanRootSink.
* Remove special-case limit-based teardown of coordinator fragment, and
  supporting functions in PlanFragmentExecutor.
* Simplified lifecycle of PlanFragmentExecutor by separating Open() into
  Open() and Exec(), the latter of which drives the sink by reading
  rows from the plan tree.
* Add child profile to PlanFragmentExecutor to measure time spent in
  each lifecycle phase.
* Removed dependency between InitExecProfiles() and starting root
  fragment.
* Removed mostly dead-code handling of LIMIT 0 queries.
* Ensured that SET returns a result set in all cases.
* Fix test_get_log() HS2 test. Errors are only guaranteed to be visible
  after fetch calls return EOS, but test was assuming this would happen
  after first fetch.

Change-Id: Ibb0064ec2f085fa3a5598ea80894fb489a01e4df
---
M be/src/exec/CMakeLists.txt
M be/src/exec/data-sink.cc
A be/src/exec/plan-root-sink.cc
A be/src/exec/plan-root-sink.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/simple-scheduler.cc
M be/src/service/fragment-exec-state.cc
M be/src/service/fragment-exec-state.h
M be/src/service/fragment-mgr.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/service/query-exec-state.h
A be/src/service/query-result-set.h
M be/src/testutil/in-process-servers.cc
M common/thrift/DataSinks.thrift
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
A fe/src/main/java/org/apache/impala/planner/PlanRootSink.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/PlannerContext.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/conjunct-ordering.test
M testdata/workloads/functional-planner/queries/PlannerTest/constant.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/data-source-tables.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/disable-preaggregations.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/distinct-estimate.test
M testdata/workloads/functional-planner/queries/PlannerTest/distinct.test
M testdata/workloads/functional-planner/queries/PlannerTest/empty.test
M testdata/workloads/functional-planner/queries/PlannerTest/hbase.test
M testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/inline-view-limit.test
M testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mem-limit-br

[Impala-ASF-CR] IMPALA-2905: Handle coordinator fragment lifecycle like all others

2016-10-15 Thread Henry Robinson (Code Review)
Hello Marcel Kornacker, Internal Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4402

to look at the new patch set (#21).

Change subject: IMPALA-2905: Handle coordinator fragment lifecycle like all 
others
..

IMPALA-2905: Handle coordinator fragment lifecycle like all others

The plan-root fragment instance that runs on the coordinator should be
handled like all others: started via RPC and run asynchronously. Without
this, the fragment requires special-case code throughout the
coordinator, and does not show up in system metrics etc.

This patch adds a new sink type, PlanRootSink, to the root fragment
instance so that the coordinator can pull row batches that are pushed by
the root instance. The coordinator signals completion to the fragment
instance via closing the consumer side of the sink, whereupon the
instance is free to complete.

Since the root instance now runs asynchronously wrt to the coordinator,
we add several coordination methods to allow the coordinator to wait for
a point in the instance's execution to be hit - e.g. to wait until the
instance has been opened.

Done in this patch:

* Add PlanRootSink
* Add coordination to PFE to allow coordinator to observe lifecycle
* Make FragmentMgr a singleton
* Removed dead code from Coordinator::Wait() and elsewhere.
* Moved result output exprs out of QES and into PlanRootSink.
* Remove special-case limit-based teardown of coordinator fragment, and
  supporting functions in PlanFragmentExecutor.
* Simplified lifecycle of PlanFragmentExecutor by separating Open() into
  Open() and Exec(), the latter of which drives the sink by reading
  rows from the plan tree.
* Add child profile to PlanFragmentExecutor to measure time spent in
  each lifecycle phase.
* Removed dependency between InitExecProfiles() and starting root
  fragment.
* Removed mostly dead-code handling of LIMIT 0 queries.
* Ensured that SET returns a result set in all cases.
* Fix test_get_log() HS2 test. Errors are only guaranteed to be visible
  after fetch calls return EOS, but test was assuming this would happen
  after first fetch.

Change-Id: Ibb0064ec2f085fa3a5598ea80894fb489a01e4df
---
M be/src/exec/CMakeLists.txt
M be/src/exec/data-sink.cc
A be/src/exec/plan-root-sink.cc
A be/src/exec/plan-root-sink.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/simple-scheduler.cc
M be/src/service/fragment-exec-state.cc
M be/src/service/fragment-exec-state.h
M be/src/service/fragment-mgr.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/service/query-exec-state.h
A be/src/service/query-result-set.h
M be/src/testutil/in-process-servers.cc
M common/thrift/DataSinks.thrift
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
A fe/src/main/java/org/apache/impala/planner/PlanRootSink.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/PlannerContext.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/conjunct-ordering.test
M testdata/workloads/functional-planner/queries/PlannerTest/constant.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/data-source-tables.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/disable-preaggregations.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/distinct-estimate.test
M testdata/workloads/functional-planner/queries/PlannerTest/distinct.test
M testdata/workloads/functional-planner/queries/PlannerTest/empty.test
M testdata/workloads/functional-planner/queries/PlannerTest/hbase.test
M testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/inline-view-limit.test
M testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mem-limit-br

[Impala-ASF-CR] IMPALA-2905: Handle coordinator fragment lifecycle like all others

2016-10-15 Thread Henry Robinson (Code Review)
Henry Robinson has posted comments on this change.

Change subject: IMPALA-2905: Handle coordinator fragment lifecycle like all 
others
..


Patch Set 21:

Rebase. Carry +2.

Fixed a bug in test_hs2::test_get_log where the test assumed that EOS would be 
returned with the first fetch() call; fixed that by calling fetch until EOS is 
returned. Bug showed up once every 40 minutes on my machine before the fix.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb0064ec2f085fa3a5598ea80894fb489a01e4df
Gerrit-PatchSet: 21
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No