[Impala-ASF-CR] IMPALA-4810: Make DECIMAL expr-test cases table driven
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
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
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
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 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
[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