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

2017-02-13 Thread 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 }} },
> as part of this patch or a follow-on?
this patch was merged before your review started, so has to be a follow on.


-- 
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-4810: Make DECIMAL expr-test cases table driven

2017-02-11 Thread Marcel Kornacker (Code Review) 
Marcel Kornacker 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 }} },
> yeah, i plan to do that before converting more cases but haven't had time y
as part of this patch or a follow-on?


-- 
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-4810: Make DECIMAL expr-test cases table driven

2017-02-10 Thread 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-4810: Make DECIMAL expr-test cases table driven

2017-02-09 Thread 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:

(3 comments)

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 }} },
> this isn't super-easy to read, how about having the v1 and v2 results on se
The next patch introduces actual differences, and lines up each pair of 
expected results:
https://gerrit.cloudera.org/#/c/5952/


Line 1345:   for (int v2 = 0; v2 <= 1; ++v2) {
> for (int v2: {0, 1}) will do the same
Thanks, added this to https://gerrit.cloudera.org/#/c/5952/


http://gerrit.cloudera.org:8080/#/c/5933/4/be/src/testutil/impalad-query-executor.h
File be/src/testutil/impalad-query-executor.h:

Line 87:   void pushExecOption(const std::string& exec_option) {
> nit: fn names are not according to our style guide
oops, i knew something didn't look right. fixed it in 
https://gerrit.cloudera.org/#/c/5952/


-- 
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-4810: Make DECIMAL expr-test cases table driven

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

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


Patch Set 4:

(2 comments)

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 }} },
this isn't super-easy to read, how about having the v1 and v2 results on 
separate lines and lined up (so they're easy to compare)? or better yet, making 
expected[1] optional, and if it's missing it's an indication that v1 and v2 
have the same result?


Line 1345:   for (int v2 = 0; v2 <= 1; ++v2) {
for (int v2: {0, 1}) will do the same


-- 
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-4810: Make DECIMAL expr-test cases table driven

2017-02-08 Thread Alex Behm (Code Review) 
Alex Behm 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/testutil/impalad-query-executor.h
File be/src/testutil/impalad-query-executor.h:

Line 87:   void pushExecOption(const std::string& exec_option) {
nit: fn names are not according to our style guide


-- 
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: Michael Ho 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

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

2017-02-07 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change.

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


Patch Set 3: Verified+1

-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

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

2017-02-07 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has submitted this change and it was merged.

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


IMPALA-4810: Make DECIMAL expr-test cases table driven

That way, we can easily run all test cases with both:
  DECIMAL_V2={false, true}

Currently, the behavior is the same, but as we work on IMPALA-4810,
the expected results will differ.

Change-Id: Ieffb2573de46bba64d1b4e8caf15cc0238a84ea1
Reviewed-on: http://gerrit.cloudera.org:8080/5933
Reviewed-by: Dan Hecht 
Tested-by: Impala Public Jenkins
---
M be/src/exprs/expr-test.cc
M be/src/testutil/impalad-query-executor.h
2 files changed, 119 insertions(+), 70 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Dan Hecht: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ieffb2573de46bba64d1b4e8caf15cc0238a84ea1
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zach Amsden

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

2017-02-07 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change.

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


Patch Set 3:

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

-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

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

2017-02-07 Thread 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 3: Code-Review+2

(1 comment)

Carry Michael's +2.

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

PS2, Line 1286: decimal_case
> nit: decimal_cases
oops. done.


-- 
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: 3
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-4810: Make DECIMAL expr-test cases table driven

2017-02-07 Thread Dan Hecht (Code Review) 
Hello Michael Ho, Zach Amsden,

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

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

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

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

IMPALA-4810: Make DECIMAL expr-test cases table driven

That way, we can easily run all test cases with both:
  DECIMAL_V2={false, true}

Currently, the behavior is the same, but as we work on IMPALA-4810,
the expected results will differ.

Change-Id: Ieffb2573de46bba64d1b4e8caf15cc0238a84ea1
---
M be/src/exprs/expr-test.cc
M be/src/testutil/impalad-query-executor.h
2 files changed, 119 insertions(+), 70 deletions(-)


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

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

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

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

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


Patch Set 2: Code-Review+2

(1 comment)

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

PS2, Line 1286: decimalCases
nit: decimal_cases


-- 
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: 2
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-4810: Make DECIMAL expr-test cases table driven

2017-02-07 Thread 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 2:

(5 comments)

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

PS1, Line 10:  DECIMAL_V2
> DECIMAL_V2
Done


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

PS1, Line 1286: decimalCases
> These are just for decimal arithmetic, right ? May be just this should be d
they are for any expression that results in a decimal type.  we can put CAST 
cases in here too.


PS1, Line 1345: ++v2
> ++v2
Done


PS1, Line 6242: enable_expr_rewr
> Please consider adding clearAllExecOptions() in ImpaladQueryExecutor too.
Done


PS1, Line 6243:   executor_->clearExecOptions();
  :   executor_->pushExecOption("ENABLE_EXPR_REWRITES=0");
  :   executor_->pushExecOption("DISABLE_CODEGEN=0");
> May be these can use your the pushExecOption();
Done


-- 
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: 2
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-4810: Make DECIMAL expr-test cases table driven

2017-02-07 Thread Dan Hecht (Code Review) 
Hello Michael Ho, Zach Amsden,

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

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

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

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

IMPALA-4810: Make DECIMAL expr-test cases table driven

That way, we can easily run all test cases with both:
  DECIMAL_V2={false, true}

Currently, the behavior is the same, but as we work on IMPALA-4810,
the expected results will differ.

Change-Id: Ieffb2573de46bba64d1b4e8caf15cc0238a84ea1
---
M be/src/exprs/expr-test.cc
M be/src/testutil/impalad-query-executor.h
2 files changed, 119 insertions(+), 70 deletions(-)


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

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

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

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

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


Patch Set 1:

(3 comments)

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

PS1, Line 10:  DECIMAL_V1
DECIMAL_V2


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

PS1, Line 6242: options.clear();
Please consider adding clearAllExecOptions() in ImpaladQueryExecutor too.


PS1, Line 6243:   options.push_back("ENABLE_EXPR_REWRITES=0");
  :   options.push_back("DISABLE_CODEGEN=0");
  :   options.push_back("EXEC_SINGLE_NODE_ROWS_THRESHOLD=0");
May be these can use your the pushExecOption();


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

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

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

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


Patch Set 1: Code-Review+1

(2 comments)

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

PS1, Line 1286: decimalCases
These are just for decimal arithmetic, right ? May be just this should be 
decimal_arithmetic_cases ?


PS1, Line 1345: v2++
++v2


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

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

2017-02-07 Thread Zach Amsden (Code Review) 
Zach Amsden has posted comments on this change.

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


Patch Set 1: Code-Review+1

Looks pretty straightforward

-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No