[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test

2017-02-20 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change.

Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
..


Patch Set 2:

(1 comment)

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

Line 1620: {{ true, 0, 9, 0 }}},
May help to add test cases which can fit in a double but still overflow when 
casting to decimal such as:

select ("5555" as DECIMAL(38,0)); //39 decimal places value


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: Yes

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

2017-02-20 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change.

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


Patch Set 15:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/benchmarks/expr-benchmark.cc
File be/src/benchmarks/expr-benchmark.cc:

PS15, Line 218: Benchmark* BenchmarkDecimalCast() {
Can you please add a sample output like other benchmark functions ?


http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/runtime/decimal-test.cc
File be/src/runtime/decimal-test.cc:

PS15, Line 169: -.1
-0.1.

Also may help to add cases which cause overflow due to rounding.

Decimal16Value::FromDouble(t3, 0.9, true, );
Decimal16Value::FromDouble(t3, 0.9, false, );


PS15, Line 224: Decimal4Value::FromDouble(t4, 0.4, true, );
Isn't a duplicate of the test above ?


http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/runtime/decimal-value.h
File be/src/runtime/decimal-value.h:

PS15, Line 52: truncating digits that
 :   /// cannot be represented or rounding if desired
rounding to the closest integer if 'round' is true. Truncate the decimal places 
otherwise.


http://gerrit.cloudera.org:8080/#/c/5951/10/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

PS10, Line 38:   if (round) {
 : // Decimal V2 behavior
 : 
 : // Multiply the double by the scale.
 : //
 : // Some quick research shows us that 10**38 in binary has 
exactly 53
 : // significant nonzero bits.  Coincidentally, double 
precision floating
 : // point gives us 53 bits for the mantissa.  TODO: validate 
that the
 : // values produced by the template are both correct, 
constant and that
 : // the multiply does not lose precision.
 : 
 : d *= DecimalUtil::GetScaleMultiplier(scale);
 : d = std::round(d);
 : const T max_value = 
DecimalUtil::GetScaleMultiplier(precision);
 : DCHECK(max_value > 0);  // no DCHECK_GT because of int128_t
 : if (UNLIKELY(std::isnan(d)) || UNLIKELY(std::abs(d) >= 
max_value)) {
 :   *overflow = true;
 :   return DecimalValue();
 : }
 : 
 : // Return the rounded integer part.
 : return DecimalValue(static_cast(d));
 :   } else {
 : // TODO: IMPALA-4924: remove DECIMAL V1 code
 : 
 : // Check overflow.
 : const T max_value = 
DecimalUtil::GetScaleMultiplier(precision - scale);
 : if (abs(d) >= max_value) {
 :   *overflow = true;
 :   return DecimalValue();
 : }
 : 
 : // Multiply the double by the scale.
 : d *= DecimalUtil::GetScaleMultiplier(scale);
 : 
> Sort of.  I didn't want to change the original at all in this change though
Are there particular cases you have in mind which may break if we use V2's 
logic for both version ?


http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

PS15, Line 132: ||
nit: long line


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

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

[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test

2017-02-20 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change.

Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
..


Patch Set 2:

(2 comments)

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

PS2, Line 1397: true
Sorry, I may be missing something. Why is this NULL ?


http://gerrit.cloudera.org:8080/#/c/6068/2/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

PS2, Line 83: abs
I think we should be consistent and use std::abs() at least in this entire file 
if not the entire code base.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4934: Disable Kudu OpenSSL initialization

2017-02-20 Thread Matthew Jacobs (Code Review) 
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4934: Disable Kudu OpenSSL initialization
..


Patch Set 4:

python tests don't handle this yet, need to wait for a patch from kudu that 
avoids DFATAL logging

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f13f3af512c6d771979638da593685524c73086
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4904,IMPALA-4914: add targeted-stress to exhaustive tests

2017-02-20 Thread Michael Brown (Code Review) 
Michael Brown has posted comments on this change.

Change subject: IMPALA-4904,IMPALA-4914: add targeted-stress to exhaustive tests
..


Patch Set 6:

http://jenkins.impala.io:8080/job/pre-review-test/21/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6bd5bbd380e636d680368e908519b042d79dfec
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3401 [DOCS] Phase 6 of "Cloudera Manager" removal

2017-02-20 Thread Laurel Hale (Code Review) 
Laurel Hale has uploaded a new change for review.

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

Change subject: IMPALA-3401 [DOCS] Phase 6 of "Cloudera Manager" removal
..

IMPALA-3401 [DOCS] Phase 6 of "Cloudera Manager" removal

Removed references to "Cloudera Manager" mostly by hiding the
paragraphs that contained the references and rewriting them
for the upstreams docs without referencing CM. This removes
the references in the rendered docs, but does not remove
them from the source XML. That part of the removal will be
done in a subsequent work item.

Change-Id: I3a5ae82ee5adfd1e2f250bc4dc26e45047dc434c
---
M docs/shared/impala_common.xml
M docs/topics/impala_breakpad.xml
M docs/topics/impala_faq.xml
M docs/topics/impala_fixed_issues.xml
M docs/topics/impala_incompatible_changes.xml
M docs/topics/impala_new_features.xml
6 files changed, 90 insertions(+), 18 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3a5ae82ee5adfd1e2f250bc4dc26e45047dc434c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Laurel Hale

[Impala-ASF-CR] IMPALA-4904,IMPALA-4914: add targeted-stress to exhaustive tests

2017-02-20 Thread Michael Brown (Code Review) 
Michael Brown has posted comments on this change.

Change subject: IMPALA-4904,IMPALA-4914: add targeted-stress to exhaustive tests
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6002/3/tests/stress/test_ddl_stress.py
File tests/stress/test_ddl_stress.py:

Line 59
> TEST_IDS (LHS L45) is equivalent to RHS L52. The RHS is simply the standard
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6bd5bbd380e636d680368e908519b042d79dfec
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4624: Implement Parquet dictionary filtering

2017-02-20 Thread Marcel Kornacker (Code Review) 
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4624: Implement Parquet dictionary filtering
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5904/8/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:

Line 515: RETURN_IF_ERROR(InitColumns(row_group_idx_, column_readers_));
> InitColumns calls AddScanRanges, so does this mean that RowGroups are read 
good point. InitColumns() needs to be split up so that the construction of the 
in-memory dictionary is done explicitly and separately from setting up the 
column readers (which probably also means that dictionaries need to be 
stand-alone constructs, not simply dangling off a column reader).

but that's a fairly involved restructuring, so let's do that in a follow-on 
patch.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a7cc3bd0523fbf3c79bd924219e909ef671cfd7
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-HasComments: Yes