You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Dan Hecht (Code Review)" <ge...@cloudera.org> on 2017/02/07 23:46:41 UTC

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

Dan Hecht has uploaded a new change for review.

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

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_V1={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, 109 insertions(+), 55 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieffb2573de46bba64d1b4e8caf15cc0238a84ea1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>

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

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>

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

Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

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

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>

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

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
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 }} },
> The next patch introduces actual differences, and lines up each pair of exp
but how about changing it so if the result is the same in both versions, you only need to specify it once? that would help readability quite a bit.


-- 
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 <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

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

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes