[Impala-ASF-CR] IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL V2

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2
..


Patch Set 4: Code-Review+2

Carry +2 forward.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL V2

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2
..


Patch Set 4:

Added decimal_v2 as a test dimension for decimal_casting.py. It's disabled for 
now as we cannot easily test decimal to decimal casting without first fixing 
'string/int/double' to decimal casting. Also added decimal to decimal casting 
to the test case now as it was missing before.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL V2

[Michael Ho (Code Review)]

Hello Dan Hecht,

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

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

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

Change subject: IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2
..

IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2

Currently, codegen supports converting type attributes (e.g.
decimal type's precision and scale, type's size) obtained via
calls to FunctionContextImpl::GetFnAttr() (previously
Expr::GetConstantInt()) in cross-compiled code to runtime constants.
This change extends this support for the query option DECIMAL_V2.

To test this change, this change also handles a subset of IMPALA-2020:
casting between decimal values is  updated to support rounding (instead
of truncation) when decimal_v2 is true.

This change also cleans up the existing code by moving the codegen
logic Expr::InlineConstant() to the codegen module and the type
related logic in Expr::GetConstantInt() to FunctionContextImpl.

Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M be/src/exprs/agg-fn-evaluator.cc
M be/src/exprs/aggregate-functions-ir.cc
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/decimal-functions-ir.cc
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/decimal-operators.h
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr.cc
M be/src/exprs/expr.h
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/decimal-value.inline.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/service/frontend.cc
M be/src/udf/udf-internal.h
M be/src/udf/udf-test-harness.cc
M be/src/udf/udf-test-harness.h
M be/src/udf/udf.cc
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
M tests/query_test/test_decimal_casting.py
29 files changed, 645 insertions(+), 491 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/5950/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL V2

[Michael Ho (Code Review)]

Hello Dan Hecht,

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

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

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

Change subject: IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2
..

IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2

Currently, codegen supports converting type attributes (e.g.
decimal type's precision and scale, type's size) obtained via
calls to FunctionContextImpl::GetFnAttr() (previously
Expr::GetConstantInt()) in cross-compiled code to runtime constants.
This change extends this support for the query option DECIMAL_V2.

To test this change, this change also handles a subset of IMPALA-2020:
casting between decimal values is  updated to support rounding (instead
of truncation) when decimal_v2 is true.

This change also cleans up the existing code by moving the codegen
logic Expr::InlineConstant() to the codegen module and the type
related logic in Expr::GetConstantInt() to FunctionContextImpl.

Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M be/src/exprs/agg-fn-evaluator.cc
M be/src/exprs/aggregate-functions-ir.cc
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/decimal-functions-ir.cc
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/decimal-operators.h
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr.cc
M be/src/exprs/expr.h
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/decimal-value.inline.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/service/frontend.cc
M be/src/udf/udf-internal.h
M be/src/udf/udf-test-harness.cc
M be/src/udf/udf-test-harness.h
M be/src/udf/udf.cc
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
M tests/query_test/test_decimal_casting.py
29 files changed, 640 insertions(+), 488 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-2020: Add rounding for decimal casts

[Zach Amsden (Code Review)]

Zach Amsden has uploaded a new patch set (#5).

Change subject: IMPALA-2020: Add rounding for decimal casts
..

IMPALA-2020: Add rounding for decimal casts

This change adds support for DECIMAL_V2 rounding behavior for both
DECIMAL to INT and DOUBLE to DECIMAL casts.  Still writing tests,
but this is ready for human eyes to look at.

Testing: In progress.  Rouding to int works as expected, rounding from
float seems to work as well.
Expect a full test suite for both modes and all edge cases to be covered.

Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr.h
M be/src/exprs/literal.cc
M be/src/runtime/decimal-test.cc
M be/src/runtime/decimal-value.h
M be/src/runtime/decimal-value.inline.h
M be/src/udf/udf.h
7 files changed, 149 insertions(+), 71 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/5951/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5951
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI
..


Patch Set 4: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I543d4d428d7240e0f710d67973867162f2fcabc8
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI
..


IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI

This commit adds heap and non-heap memory usage of the embedded
JVM in the memory metrics and exposes these metrics in /memz web page of
the impalad and catalog web UI.

Change-Id: I543d4d428d7240e0f710d67973867162f2fcabc8
Reviewed-on: http://gerrit.cloudera.org:8080/5909
Reviewed-by: Dimitris Tsirogiannis 
Tested-by: Impala Public Jenkins
---
M be/src/util/default-path-handlers.cc
M fe/src/main/java/org/apache/impala/common/JniUtil.java
M www/memz.tmpl
3 files changed, 110 insertions(+), 8 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Dimitris Tsirogiannis: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I543d4d428d7240e0f710d67973867162f2fcabc8
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-4897: AnalysisException: specified cache pool does not exist

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4897: AnalysisException: specified cache pool does not 
exist
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5972/1/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:

PS1, Line 258: if IS_HDFS:
 :   self.run_test_case('QueryTest/alter-table-hdfs-caching', 
vector,
 :   use_db=unique_database, 
multiple_impalad=self._use_multiple_impalad(vector))
> I think it may be best to put it in test_hdfs_caching.py? What do you think
I thought about that but the goal of this test is to test an ALTER TABLE 
functionality and the hdfs caching part is not the core of the test, which is 
why I left it here instead.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefe61556bc28ae320f3f41fdc930d37b258d970a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4897: AnalysisException: specified cache pool does not exist

[Dimitris Tsirogiannis (Code Review)]

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4897: AnalysisException: specified cache pool does not 
exist
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5972/1/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:

PS1, Line 258: if IS_HDFS:
 :   self.run_test_case('QueryTest/alter-table-hdfs-caching', 
vector,
 :   use_db=unique_database, 
multiple_impalad=self._use_multiple_impalad(vector))
I think it may be best to put it in test_hdfs_caching.py? What do you think?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefe61556bc28ae320f3f41fdc930d37b258d970a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4897: AnalysisException: specified cache pool does not exist

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

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

Change subject: IMPALA-4897: AnalysisException: specified cache pool does not 
exist
..

IMPALA-4897: AnalysisException: specified cache pool does not exist

A few tests were added with IMPALA-1670 that made use of HDFS caching.
This patch moves these tests to a new file and only executes them
when the default filesystem is HDFS.

Also added a testcase for testing alter table ADD multiple PARTITIONS
for non-HDFS filesystems.

Change-Id: Iefe61556bc28ae320f3f41fdc930d37b258d970a
---
A 
testdata/workloads/functional-query/queries/QueryTest/alter-table-hdfs-caching.test
M testdata/workloads/functional-query/queries/QueryTest/alter-table.test
M tests/metadata/test_ddl.py
3 files changed, 123 insertions(+), 65 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iefe61556bc28ae320f3f41fdc930d37b258d970a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4905: Reduce coordinator lock contention in RPC handler

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4905: Reduce coordinator lock contention in RPC handler
..


Patch Set 1:

(1 comment)

Changed how the logging works - it didn't quite work in the previous patch 
because WaitForAllInstance() isn't called until late on in query execution.

http://gerrit.cloudera.org:8080/#/c/5971/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS1, Line 829:   for (const InstanceState* state: fragment_instance_states_) {
 : files_to_move.insert(
 : state->insert_status().files_to_move.begin(),
 : state->insert_status().files_to_move.end());
 : 
 : for (const PartitionStatusMap::value_type& partition:
 :  state->insert_status().per_partition_status) {
 :   TInsertPartitionStatus* status = 
&(per_partition_status_[partition.first]);
 :   status->__set_num_modified_rows(
 :   status->num_modified_rows + 
partition.second.num_modified_rows);
 :   status->__set_kudu_latest_observed_ts(std::max(
 :   partition.second.kudu_latest_observed_ts, 
status->kudu_latest_observed_ts));
 :   status->__set_id(partition.second.id);
 :   
status->__set_partition_base_dir(partition.second.partition_base_dir);
 : 
 :   if (partition.second.__isset.stats) {
 : if (!status->__isset.stats) 
status->__set_stats(TInsertStats());
 : DataSink::MergeDmlStats(partition.second.stats, 
>stats);
 :   }
 : }
 :   }
> I expect that large INSERT queries will take a while longer to run because 
I don't think it's going to be excessive, even though this is an N^2 loop (the 
metadata update times usually dominate). Worth checking to see if it makes a 
difference though.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7599780785c4e9306711f535bf4726a247873e2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4905: Reduce coordinator lock contention in RPC handler

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new patch set (#2).

Change subject: IMPALA-4905: Reduce coordinator lock contention in RPC handler
..

IMPALA-4905: Reduce coordinator lock contention in RPC handler

Fragment instances call ReportExecStatus every few seconds, so there are
often very many of those RPCs in flight. The handler in
Coordinator::UpdateFragmentExecStatus() would try and take the shared
coordinator lock twice, once to merge in any insert file move operations
to be performed, and once to print all the other instance state's
status.

There was also a bug where the insert-merge branch would always be
taken (and so would the lock) even if the query was not an INSERT
statement.

This patch refactors UpdateFragmentExecStatus() so that it never takes
the coordinator lock. Instead, the insert updates are saved in the
fragment instance state to be merged during finalization. The logging is
now done without taking the coordinator lock unnecessarily, and also
avoid taking the fragment instance state locks since done() can be read
safely without synchronization on x86.

Change-Id: Id7599780785c4e9306711f535bf4726a247873e2
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/fragment-instance-state.cc
3 files changed, 61 insertions(+), 44 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7599780785c4e9306711f535bf4726a247873e2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4370: Divide and modulo result types for DECIMAL version V2

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4370: Divide and modulo result types for DECIMAL version 
V2
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5952/6/fe/src/main/java/org/apache/impala/catalog/ScalarType.java
File fe/src/main/java/org/apache/impala/catalog/ScalarType.java:

PS6, Line 115: precision
> Yeah I thought about adding a check for that but it wasn't entirely clear t
The answer is that it can be > 38. Negative is a parser error (not an integral 
literal) but > 38 is an analysis error (see TypeDef.analyzeScalarType()).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I83e7f7787edfa4b4bddc25945090542a0e90881b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4905: Reduce coordinator lock contention in RPC handler

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new change for review.

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

Change subject: IMPALA-4905: Reduce coordinator lock contention in RPC handler
..

IMPALA-4905: Reduce coordinator lock contention in RPC handler

Fragment instances call ReportExecStatus every few seconds, so there are
often very many of those RPCs in flight. The handler in
Coordinator::UpdateFragmentExecStatus() would try and take the shared
coordinator lock twice, once to merge in any insert file move operations
to be performed, and once to print all the other instance state's
status.

There was also a bug where the insert-merge branch would always be
taken (and so would the lock) even if the query was not an INSERT
statement.

This patch refactors UpdateFragmentExecStatus() so that it never takes
the coordinator lock. Instead, the insert updates are saved in the
fragment instance state to be merged during finalization, and the
logging is now done by the main coordinator thread which is signalled by
the RPC handler.

Change-Id: Id7599780785c4e9306711f535bf4726a247873e2
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/fragment-instance-state.cc
3 files changed, 77 insertions(+), 50 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id7599780785c4e9306711f535bf4726a247873e2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 

[Impala-ASF-CR] IMPALA-4370: Divide and modulo result types for DECIMAL version V2

[Dan Hecht (Code Review)]

Dan Hecht has uploaded a new patch set (#7).

Change subject: IMPALA-4370: Divide and modulo result types for DECIMAL version 
V2
..

IMPALA-4370: Divide and modulo result types for DECIMAL version V2

Implement the new DECIMAL return type rules for divide and modulo
expressions, active when query option DECIMAL_V2=1. See the comment
in the code for more details. A couple of examples that show why new
return type rules for divide are desirable.

For modulo, the return types are actually equivalent, though the
rules are expressed differently to have consistency with how
precision fixups are handled for each version.

DECIMAL Version 1:

+---+
| cast(1 as decimal(20,0)) / cast(3 as decimal(20,0)) |
+-+
| 0   |
+---+

DECIMAL Version 2:

+---+
| cast(1 as decimal(20,0)) / cast(3 as decimal(20,0)) |
+-+
| 0.33|
+---+

DECIMAL Version 1:

+---+
| cast(1 as decimal(6,0)) / cast(0.1 as decimal(38,38)) |
+---+
| NULL  |
+---+
WARNINGS: UDF WARNING: Expression overflowed, returning NULL

DECIMAL Version 2:

+---+
| cast(1 as decimal(6,0)) / cast(0.1 as decimal(38,38)) |
+---+
| 10.00 |
+---+

Change-Id: I83e7f7787edfa4b4bddc25945090542a0e90881b
---
M be/src/exprs/expr-test.cc
M be/src/runtime/decimal-value.inline.h
M be/src/testutil/impalad-query-executor.h
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M tests/query_test/test_decimal_queries.py
10 files changed, 325 insertions(+), 75 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/5952/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I83e7f7787edfa4b4bddc25945090542a0e90881b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] Fix merge conflict

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: Fix merge conflict
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5444d6b44b9aeea18f7861849513a2bde5c881f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] Fix merge conflict

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: Fix merge conflict
..


Fix merge conflict

Commits fcc2d81 and 1335af3 conflicted.

Change-Id: Ia5444d6b44b9aeea18f7861849513a2bde5c881f
Reviewed-on: http://gerrit.cloudera.org:8080/5967
Reviewed-by: Bharath Vissapragada 
Reviewed-by: Dimitris Tsirogiannis 
Tested-by: Impala Public Jenkins
---
M common/thrift/generate_error_codes.py
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Impala Public Jenkins: Verified
  Bharath Vissapragada: Looks good to me, but someone else must approve
  Dimitris Tsirogiannis: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia5444d6b44b9aeea18f7861849513a2bde5c881f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-4810: Make DECIMAL expr-test cases table driven

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4810: Make DECIMAL expr-test cases table driven
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5933/4/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 1288: {{ false, 2230, 5, 3 }, { false, 2230, 5, 3 }} },
> but how about changing it so if the result is the same in both versions, yo
yeah, i plan to do that before converting more cases but haven't had time yet.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieffb2573de46bba64d1b4e8caf15cc0238a84ea1
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL V2

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2
..


Patch Set 2:

Verified there is no perf regression with decimal_v2=false using following 
query:

select sum(cast(l_quantity as DECIMAL(12,1)) + cast(l_extendedprice as 
DECIMAL(12,1)) + cast(l_discount as DECIMAL(12,1)) + cast(l_tax as 
DECIMAL(12,1))) from tpch20_parquet.lineitem;

Avg: 2.23s

With decimal_v2 to true, the query slowed down quite a bit:

Avg: 4.53s

Verified that the optimized IR already had constant propagated.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new change for review.

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

Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
..

IMPALA-4546: Fix Moscow timezone conversion after 2014

In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.

Testing:
Added BE Expr tests.

Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 107 insertions(+), 42 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-4828: Alter Kudu schema outside Impala may crash on read

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-4828: Alter Kudu schema outside Impala may crash on read
..


Patch Set 6: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6d43f5bb9811e728ad592933066d006c8fb4553a
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4828: Alter Kudu schema outside Impala may crash on read

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4828: Alter Kudu schema outside Impala may crash on read
..


Patch Set 6: Code-Review+1

carrying Sailesh's +1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6d43f5bb9811e728ad592933066d006c8fb4553a
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4828: Alter Kudu schema outside Impala may crash on read

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4828: Alter Kudu schema outside Impala may crash on read
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5840/5//COMMIT_MSG
Commit Message:

PS5, Line 22: Kudu scanner
> nit: I got confused thinking it was our KuduScanner. It's happening on the 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6d43f5bb9811e728ad592933066d006c8fb4553a
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4828: Alter Kudu schema outside Impala may crash on read

[Matthew Jacobs (Code Review)]

Hello Sailesh Mukil,

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

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

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

Change subject: IMPALA-4828: Alter Kudu schema outside Impala may crash on read
..

IMPALA-4828: Alter Kudu schema outside Impala may crash on read

Creating a table in Impala, changing the column schema
outside of Impala, and then reading again in Impala may
result in a crash. Impala may attempt to dereference
pointers that aren't there. This happens if a string column
is dropped and then a new, non string column is added with
the old string column's name.

The Kudu scan token contains the projection schema, and that
is validated when opening the Kudu scanner, but the issue is
that during planning, Impala assumes the types of columns
haven't changed when creating the scan tokens. This is fixed
by adding a check when creating the scan token, and failing
the query if the column types changed. Impala then relies on
the Kudu client to properly validate that the underlying
schema is still represented by the scan token, and that
deserialization will fail if it no longer matches. A test
case was added for this particular crash scenario, which now
fails during planning as expected. This does not attempt to
validate the Kudu client validation at deserialization time,
though that would be valuable coverage to add in the future.

Columns being removed don't produce a crash; the query fails
gracefully. A test was added for this case.

Columns being added should not affect this scenario, but a
test was added anyway.

Change-Id: I6d43f5bb9811e728ad592933066d006c8fb4553a
---
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M tests/query_test/test_kudu.py
2 files changed, 116 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/5840/6
-- 
To view, visit http://gerrit.cloudera.org:8080/5840
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6d43f5bb9811e728ad592933066d006c8fb4553a
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-4828: Alter Kudu schema outside Impala may crash on read

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4828: Alter Kudu schema outside Impala may crash on read
..


Patch Set 5: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5840/5//COMMIT_MSG
Commit Message:

PS5, Line 22: Kudu scanner
nit: I got confused thinking it was our KuduScanner. It's happening on the Kudu 
side right? If so, maybe change this to "Kudu's client scanner"?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6d43f5bb9811e728ad592933066d006c8fb4553a
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3410 [DOCS] Rework Impala authentication topics to be generic

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-3410 [DOCS] Rework Impala authentication topics to be 
generic
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5962/2/docs/topics/impala_kerberos.xml
File docs/topics/impala_kerberos.xml:

PS2, Line 46: KDC
Not defined yet


Line 50: 
Can this be removed?


http://gerrit.cloudera.org:8080/#/c/5962/2/docs/topics/impala_ldap.xml
File docs/topics/impala_ldap.xml:

Line 189: Specify the option on the impalad command 
line.
This paragraph is confusing now.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I639a55eb43555cf074c26d23b5c72f778073231c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Laurel Hale 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3411 [DOCS] Rework Impala governance topics to be generic.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-3411 [DOCS] Rework Impala governance topics to be 
generic.
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5957/1//COMMIT_MSG
Commit Message:

Line 12: to use cluster management software with a focus on governance.
Why not always remove them?


http://gerrit.cloudera.org:8080/#/c/5957/1/docs/topics/impala_lineage.xml
File docs/topics/impala_lineage.xml:

PS1, Line 60: Use a cluster manager with enhanced governance capabilities to 
transform
:   lineage data generated by Impala into graphs for easy 
visualization.
This feels off-topic to me.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I192bc2d1de89e55418c045d1a0e5433cf02cf782
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Laurel Hale 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 12:

> I'm not sure the extra complexity of the UI is worth it.

Without the UI, we'd have to remember how to construct the URLs and what the 
arguments are. The only complexity the UI adds is log_level.tmpl which handles 
presentation of the returned data. Everything else would stay roughly the same 
if we had a command-line-only interface.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL V2

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2
..


Patch Set 2: Code-Review+2

(2 comments)

Please take a look at whether it makes sense to add coverage in 
test_decimal_casting.py before committing. Also, please see my question earlier 
about whether perf (for v2=false) was checked.

http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 1334: {{ false, 9, 9, 0 }, { true, 0, 9, 0 }} },
> Modified the test above to show rounding of 0.12344 vs 0.12345. Is that wha
Yes


http://gerrit.cloudera.org:8080/#/c/5950/1/testdata/workloads/functional-query/queries/QueryTest/decimal.test
File testdata/workloads/functional-query/queries/QueryTest/decimal.test:

Line 469:  RESULTS
> The query above was run with v2=false. Did you mean something else ?
Yes. Somehow I didn't see that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL V2

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2
..


Patch Set 1:

We also have test_decimal_casting.py. I haven't looked at it in detail but 
maybe there's more coverage you can get there (or adjustments you need to make 
for rounding).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] Fix merge conflict

[Dimitris Tsirogiannis (Code Review)]

Dimitris Tsirogiannis has posted comments on this change.

Change subject: Fix merge conflict
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5444d6b44b9aeea18f7861849513a2bde5c881f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] Fix merge conflict

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: Fix merge conflict
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5444d6b44b9aeea18f7861849513a2bde5c881f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] Fix merge conflict

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: Fix merge conflict
..


Patch Set 1:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/260/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5444d6b44b9aeea18f7861849513a2bde5c881f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] Fix merge conflict

[Dan Hecht (Code Review)]

Dan Hecht has uploaded a new change for review.

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

Change subject: Fix merge conflict
..

Fix merge conflict

Commits fcc2d81 and 1335af3 conflicted.

Change-Id: Ia5444d6b44b9aeea18f7861849513a2bde5c881f
---
M common/thrift/generate_error_codes.py
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia5444d6b44b9aeea18f7861849513a2bde5c881f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 

[Impala-ASF-CR] IMPALA-4263: Fix wrong ommission of agg/analytic hash exchanges.

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4263: Fix wrong ommission of agg/analytic hash exchanges.
..


Patch Set 1: Code-Review+2

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5774/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:

Line 799: if (!childFragment.refsNullableTupleId(partitionExprs)) {
combine with preceding if


http://gerrit.cloudera.org:8080/#/c/5774/1/fe/src/main/java/org/apache/impala/planner/PlanFragment.java
File fe/src/main/java/org/apache/impala/planner/PlanFragment.java:

Line 386:* Returns true if 'exprs' reference a tuple that is made nullable 
in this fragment.
"in this fragment but not in any of its input fragments"


Line 390: List groupingExprTids = Lists.newArrayList();
remove the reference to grouping, it's overly specific.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I121222179378e56836422a69451d840a012c9e54
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 12:

> Marcel, you can make changes like you described just with a URL,
 > see test_web_pages.py which does just that. The UI is nice because
 > then you also have a way of inspecting the state of the system.
 > Allowing mutation without having a way to inspect the state seems
 > bad for usability (and esp. if we expose it to users).

I didn't suggest not allowing inspection of the existing log levels, which can 
also be done via a url.

I'm not sure the extra complexity of the UI is worth it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL V2

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2
..


Patch Set 1:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/5950/1//COMMIT_MSG
Commit Message:

Line 21: 
> also mention that this does part of IMPALA-2020 for decimal->decimal casts.
Done


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

Line 468:   boost::unordered_set* symbols);
> never mind, i think i saw this. it's only used in one place and it just mov
Yes. This change allows CreateFrom* to be all private functions of LlvmCodeGen.


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:

PS1, Line 1740: output_type
> or, after seeing the full patch, i think you can just revert this to avoid 
Reverted and merged with Tim's patch.


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:

PS1, Line 478: output_precision < val_precision
> this gets codegen'ed away, right?
Yes, it will.


Line 479:   abs(result.val16) >=  
DecimalUtil::GetScaleMultiplier(output_precision)) {
> how about using RETURN_IF_OVERFLOW() so we get the warning (and later it ma
Done


PS1, Line 505: static_cast(is_decimal_v2)
> usually we convert to bool like this: decimal_v2 != 0
Done


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/exprs/expr-codegen-test.cc
File be/src/exprs/expr-codegen-test.cc:

Line 310:   int64_t v = 125;
> what's this?
Oh, it's just verifying the rounding actually occurred with decimal_v2.


PS1, Line 316: 13
> and this?
Comments added.


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 1334: {{ false, -9, 9, 0 }, { true, 0, 9, 0 }} },
> how about a positive/negative test where the digit is 5 (to show round-up)?
Modified the test above to show rounding of 0.12344 vs 0.12345. Is that what 
you mean ?


PS1, Line 1357: v2
> sorry about that
No problem. I missed that in the review too.


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/udf/udf-internal.h
File be/src/udf/udf-internal.h:

PS1, Line 131: attributes of the return type and argument types of a UDF/UDA
> static attributes of the UDF/UDA that can be injected as constants by codeg
Done


PS1, Line 148:  This function returns the various attributes of the return type 
and argument types
 :   /// recorded in 'ctx'
 :   /// It also returns the value of the query option decimal_v2 
which dictates the behavior
 :   /// of cast to Decimal type
> How about just say
Done


PS1, Line 160:  /// Functions which implement the UDF or UDA interface should 
use this function so that
 :   /// runtime constants of the type attributes would be inlined 
when codegen is enabled.
> this can be deleted if you use the wording above (and it's good to mention 
Done


Line 170:   /// - whether query option decimal_v2 is set (returns 0 or 1)
> could just refer to ConstFnAttr.
Done


http://gerrit.cloudera.org:8080/#/c/5950/1/testdata/workloads/functional-query/queries/QueryTest/decimal.test
File testdata/workloads/functional-query/queries/QueryTest/decimal.test:

Line 469:  RESULTS
> how about running this query with v2=false as well so the difference is cle
The query above was run with v2=false. Did you mean something else ?


Line 495: 
> this is going to conflict with my change, which causes us to run decimal.te
Yes, we should coordinate. If your change merges first, I will move it over to 
the new file which doesn't exist yet in trunk.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL V2

[Michael Ho (Code Review)]

Michael Ho has uploaded a new patch set (#2).

Change subject: IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2
..

IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2

Currently, codegen supports converting type attributes (e.g.
decimal type's precision and scale, type's size) obtained via
calls to FunctionContextImpl::GetFnAttr() (previously
Expr::GetConstantInt()) in cross-compiled code to runtime constants.
This change extends this support for the query option DECIMAL_V2.

To test this change, this change also handles a subset of IMPALA-2020:
casting between decimal values is  updated to support rounding (instead
of truncation) when decimal_v2 is true.

This change also cleans up the existing code by moving the codegen
logic Expr::InlineConstant() to the codegen module and the type
related logic in Expr::GetConstantInt() to FunctionContextImpl.

Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M be/src/exprs/agg-fn-evaluator.cc
M be/src/exprs/aggregate-functions-ir.cc
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/decimal-functions-ir.cc
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/decimal-operators.h
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr.cc
M be/src/exprs/expr.h
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/decimal-value.inline.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/service/frontend.cc
M be/src/udf/udf-internal.h
M be/src/udf/udf-test-harness.cc
M be/src/udf/udf-test-harness.h
M be/src/udf/udf.cc
M common/thrift/generate_error_codes.py
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
29 files changed, 605 insertions(+), 463 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht