[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. IMPALA-10950: Update expr-benchmark.cc With the introduction of PlanRootSink by IMPALA-2905, query planner has moved the scalar expression's thrift definition from 'fragments[0].output_sink.output_exprs' to 'fragments[0].plan.nodes[0].union_node.const_expr_lists[0]' for a constant query. This patch adjusts expr-benchmark.cc to generate the ScalarExpr from the right thrift location and also modify the helper class to ensure proper resource cleanup at the end of the benchmark. We explicitly set ENABLE_EXPR_REWRITES=0 to prevent expression rewrite by FoldConstantsRule.java. The benchmark used to run without codegen. This patch modifies the benchmark to run a benchmark suite both with and without codegen. Testing: - Run and verify that expr-benchmark does not crash. Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Reviewed-on: http://gerrit.cloudera.org:8080/17894 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/benchmarks/expr-benchmark.cc 1 file changed, 631 insertions(+), 312 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 8 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 7: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 7 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Sat, 09 Oct 2021 02:23:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7526/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 7 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 08 Oct 2021 20:16:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 7 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 08 Oct 2021 20:16:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 6 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 08 Oct 2021 20:13:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Riza Suminto has posted comments on this change. (
http://gerrit.cloudera.org:8080/17894 )
Change subject: IMPALA-10950: Update expr-benchmark.cc
..
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc
File be/src/benchmarks/expr-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@108
PS3, Line 108: FragmentState* fragment_state = state->obj_pool()->Add(
> I'll add a query plan sample as a comment.
Done
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc
File be/src/benchmarks/expr-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@78
PS4, Line 78: ity class
> Will do.
Done
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@710
PS4, Line 710: 219
> Good to know. Thanks!
Done
http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc
File be/src/benchmarks/expr-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc@104
PS5, Line 104: pool_.Add(new RuntimeState(query_ctx, &e
> Ack, I'll try it in the next patch set.
Done
http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc@171
PS5, Line 171: gen) {
> Since I remove the test data vector and ReleaseTestData() method from Plann
In patch set 6, I register new ExprTestData declaration with pool_. So it
should be cleaned up when pool_ is cleaned.
--
To view, visit http://gerrit.cloudera.org:8080/17894
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f
Gerrit-Change-Number: 17894
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Thu, 07 Oct 2021 15:07:09 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/9573/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 6 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Thu, 07 Oct 2021 15:06:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Hello Qifan Chen, Gabor Kaszab, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17894 to look at the new patch set (#6). Change subject: IMPALA-10950: Update expr-benchmark.cc .. IMPALA-10950: Update expr-benchmark.cc With the introduction of PlanRootSink by IMPALA-2905, query planner has moved the scalar expression's thrift definition from 'fragments[0].output_sink.output_exprs' to 'fragments[0].plan.nodes[0].union_node.const_expr_lists[0]' for a constant query. This patch adjusts expr-benchmark.cc to generate the ScalarExpr from the right thrift location and also modify the helper class to ensure proper resource cleanup at the end of the benchmark. We explicitly set ENABLE_EXPR_REWRITES=0 to prevent expression rewrite by FoldConstantsRule.java. The benchmark used to run without codegen. This patch modifies the benchmark to run a benchmark suite both with and without codegen. Testing: - Run and verify that expr-benchmark does not crash. Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f --- M be/src/benchmarks/expr-benchmark.cc 1 file changed, 631 insertions(+), 312 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/17894/6 -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 6 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@108 PS3, Line 108: FragmentState* fragment_state = state->obj_pool()->Add( > can you add context here about the kind of plan generated? maybe just the t I'll add a query plan sample as a comment. http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@78 PS4, Line 78: ity class > nit: how about EnableCodegen(true) Will do. http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@629 PS4, Line 629: col", > was the previous case not valid? Right. This expression extract PROTOCOL, but the previous url does not have one, and the expression return an error. http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc@104 PS5, Line 104: new RuntimeState(query_ctx, &exec_env_); > you can also allocate this from pool_ so that it can be cleaned eventually Ack, I'll try it in the next patch set. http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc@171 PS5, Line 171: test_data > does this ever get deleted? Since I remove the test data vector and ReleaseTestData() method from Planner, this will stay until the benchmark program ends. There is no crash though, because the destructor of ExprTestData will make sure to close all the objects when benchmark program ends. -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Thu, 07 Oct 2021 01:48:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@108 PS3, Line 108: FragmentState* fragment_state = state->obj_pool()->Add( > You're absolutely right! We explicitly set ENABLE_EXPR_REWRITES=0 in patch can you add context here about the kind of plan generated? maybe just the textual plan itself? http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@78 PS4, Line 78: ity class nit: how about EnableCodegen(true) http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@629 PS4, Line 629: col", was the previous case not valid? http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc@104 PS5, Line 104: new RuntimeState(query_ctx, &exec_env_); you can also allocate this from pool_ so that it can be cleaned eventually http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc@171 PS5, Line 171: test_data does this ever get deleted? -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Thu, 07 Oct 2021 01:35:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 5: Code-Review+1 (1 comment) Looks great! http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@710 PS4, Line 710: 173 > The benchmark measure how many iters/ms can be done to evaluate a ScalarExp Good to know. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Wed, 06 Oct 2021 23:46:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Riza Suminto has posted comments on this change. (
http://gerrit.cloudera.org:8080/17894 )
Change subject: IMPALA-10950: Update expr-benchmark.cc
..
Patch Set 5:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/17894/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17894/4//COMMIT_MSG@19
PS4, Line 19: ru
> nit. remove?
Done
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc
File be/src/benchmarks/expr-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@141
PS3, Line 141: private:
: Frontend frontend_;
: ExecEnv exec_env_;
:
> Since ScalarExpEvaluator now maintain resources, I was thinking that we sho
Actually, I decide to remove this in patch set 5.
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc
File be/src/benchmarks/expr-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@51
PS4, Line 51: // Struct holding reference to RuntimeState, FragmentState,
ScalarExpr, and
> May need to add some description on this struct. TestData may be named as E
Done
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@52
PS4, Line 52: / ScalarExprEvaluator for a specific expression.
: // This ExprTestData should be obtained through
Planner::PrepareScalarExpression() that
: // will populate all the objects and open the ScalarExprEvaluator.
: struct ExprTestData {
> Should define a constructor to init these data members.
Done
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@81
PS4, Line 81: public:
> nit. May add comment: *test_data contains valid TestData in which expr is a
Done
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@84
PS4, Line 84: ec_env_.In
> I wonder if there is even a need to have this vector.
Removed.
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@87
PS4, Line 87:
: // Tell planner to enable/disable codegen on
PrepareScalarExpression.
: void SetCodegen(bool enable) {
query_options_.__set_disable_codegen(!enable); }
:
: // Create ExprTestData for a given query.
> nit. May use TQueryCtx cstr to populate query_ctx.
I think TQueryCtx only has empty constructor. But I tidy this up a bit so it is
shorter. Hope it looks better.
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@96
PS4, Line 96: TQueryCtx q
> Better to prepare these arguments first and then call the construct with th
Done
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@710
PS4, Line 710: 173
> I wonder why the numbers in 10%ile, 50%ile and 90%ile column are larger in
The benchmark measure how many iters/ms can be done to evaluate a
ScalarExprEvaluator 256 times (this is driven by BenchmarkQueryFn).
The codegen version often has much higher iters/ms because it is faster. I
think it does not have reinterpret_cast and the function call path is shorter
that the non-codegen.
--
To view, visit http://gerrit.cloudera.org:8080/17894
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f
Gerrit-Change-Number: 17894
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Wed, 06 Oct 2021 21:44:30 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/9571/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Wed, 06 Oct 2021 21:41:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Hello Qifan Chen, Gabor Kaszab, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17894 to look at the new patch set (#5). Change subject: IMPALA-10950: Update expr-benchmark.cc .. IMPALA-10950: Update expr-benchmark.cc With the introduction of PlanRootSink by IMPALA-2905, query planner has moved the scalar expression's thrift definition from 'fragments[0].output_sink.output_exprs' to 'fragments[0].plan.nodes[0].union_node.const_expr_lists[0]' for a constant query. This patch adjusts expr-benchmark.cc to generate the ScalarExpr from the right thrift location and also modify the helper class to ensure proper resource cleanup at the end of the benchmark. We explicitly set ENABLE_EXPR_REWRITES=0 to prevent expression rewrite by FoldConstantsRule.java. The benchmark used to run without codegen. This patch modifies the benchmark to run a benchmark suite both with and without codegen. Testing: - Run and verify that expr-benchmark does not crash. Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f --- M be/src/benchmarks/expr-benchmark.cc 1 file changed, 616 insertions(+), 312 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/17894/5 -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Qifan Chen has posted comments on this change. (
http://gerrit.cloudera.org:8080/17894 )
Change subject: IMPALA-10950: Update expr-benchmark.cc
..
Patch Set 4:
(8 comments)
Looks good to me. Just have some minor comments.
http://gerrit.cloudera.org:8080/#/c/17894/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17894/4//COMMIT_MSG@19
PS4, Line 19: be
nit. remove?
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc
File be/src/benchmarks/expr-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@51
PS4, Line 51: struct TestData {
May need to add some description on this struct. TestData may be named as
ExprTestData to make it clear.
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@52
PS4, Line 52: RuntimeState* state;
: FragmentState* fragment_state;
: ScalarExpr* expr;
: ScalarExprEvaluator* eval;
Should define a constructor to init these data members.
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@81
PS4, Line 81: // Assumes this is a constant query.
nit. May add comment: *test_data contains valid TestData in which expr is
already opened.
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@84
PS4, Line 84: test_data_
I wonder if there is even a need to have this vector.
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@87
PS4, Line 87: query_ctx.client_request.stmt = query;
: query_ctx.client_request.query_options = query_options_;
: query_ctx.client_request.query_options.disable_codegen =
!codegen_;
: query_ctx.client_request.query_options.enable_expr_rewrites =
false;
: query_ctx.__set_session(session_state_);
nit. May use TQueryCtx cstr to populate query_ctx.
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@96
PS4, Line 96: test->state
Better to prepare these arguments first and then call the construct with them.
The code will look better.
ExprTestData test(state, fragment, expr, eval);
http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@710
PS4, Line 710: 193
I wonder why the numbers in 10%ile, 50%ile and 90%ile column are larger in
codegen version than the non-codegen version.
--
To view, visit http://gerrit.cloudera.org:8080/17894
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f
Gerrit-Change-Number: 17894
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Wed, 06 Oct 2021 18:16:30 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG@9 PS3, Line 9: With the introduction of PlanRootSink by IMPALA-2905, query planner has : moved the scalar expression's thrift definition from : 'fragments[0].output_sink.output_exprs' to > which change made this switch? curious as to what the context there was This change was initiated by IMPALA-2905 by adding PlanRootSink. I add this explanation in the commit message. http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG@16 PS3, Line 16: sion rewrite by : FoldConstantsRule.java. > why are they now crashing if the benchmark earlier used to run fine without My bad. I'm not aware that ScalarExprEvaluator::Open() need to be called first. Otherwise, its fn_ctx->impl()->staging_input_vals() will be empty and crash the interpreted function call path. Patch set 4 fix this. http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@108 PS3, Line 108: vector texprs = union_node.const_expr_lists[0]; > you can use this to query option to disable re-write: ENABLE_EXPR_REWRITES You're absolutely right! We explicitly set ENABLE_EXPR_REWRITES=0 in patch set 4 to avoid expression rewrite. http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@119 PS3, Line 119: RETURN_IF_ERROR(test->fragment_state->CreateCodegen()); : LlvmCodeGen* codegen = test->fragment_state->codegen(); : DCHECK(codegen != NULL); : RETURN_IF_ERROR(test-> > do these need to be removed? Done http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@134 PS3, Line 134: ol_.Clear(); > why are we disabling this? Sorry, this was blindly copied from fe-support.cc. Patch set 4 enable the optimization. http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@141 PS3, Line 141: vector> test_data_; : : TQueryOptions query_options_; : T > nit: you can probably just add this to the destructor instead of a separate Since ScalarExpEvaluator now maintain resources, I was thinking that we should clean up all dangling resources once a benchmark suite measurement has finished. Patch set 4 add another call to this function right after Beanchmark::Measure() call. I tried run the benchmark with and without calling ReleaseTestData() after measurement. The measurement result between the two, however, does not seem to differ much. But considering we're running more benchmark suite and expressions, I think cleaning up after measurement is the right thing to do. Please let me know what you think. http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@183 PS3, Line 183: data->dummy_result += reinterpret_cast(value); : } > we should probably do both with and without codegen and also keep the bench Done. Controlled through Planner::SetCodegen(). http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@234 PS3, Line 234: // (relative) (relative) (relative) : // - : // equals254 255 257 1X 1X 1X : // not equals320 322 324 1.26X 1.26X 1.26X : // strstr106 107 107 0.419X 0.419X 0.418X : //strncmp1215 216 217 0.848X 0.847X 0.845X : //strncmp2209 211 211 0.825X 0.825X 0.822X : //strncmp3254 256 257 1X 1X 1X : // regex 28.1 28.2 28.3 0.111X 0.11X 0.11X : // : // LikeCodegen: Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile : // > all of these values are for benchmark without codegen, can you update these Done -- To view, visit http://gerrit.cloudera.org:8080/178
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/9564/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 4 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Wed, 06 Oct 2021 05:39:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Hello Gabor Kaszab, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17894 to look at the new patch set (#4). Change subject: IMPALA-10950: Update expr-benchmark.cc .. IMPALA-10950: Update expr-benchmark.cc With the introduction of PlanRootSink by IMPALA-2905, query planner has moved the scalar expression's thrift definition from 'fragments[0].output_sink.output_exprs' to 'fragments[0].plan.nodes[0].union_node.const_expr_lists[0]' for a constant query. This patch adjusts expr-benchmark.cc to generate the ScalarExpr from the right thrift location and also modify the helper class to ensure proper resource cleanup at the end of the benchmark. We explicitly set ENABLE_EXPR_REWRITES=0 to prevent expression rewrite by FoldConstantsRule.java. The benchmark used to be run without codegen. This patch modifies the benchmark to run a benchmark suite both with and without codegen. Testing: - Run and verify that expr-benchmark does not crash. Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f --- M be/src/benchmarks/expr-benchmark.cc 1 file changed, 611 insertions(+), 307 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/17894/4 -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 4 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@108 PS3, Line 108: const TQueryExecRequest& query_request = request.query_exec_request; > This one seems to be a bigger obstacle for benchmarking expression. you can use this to query option to disable re-write: ENABLE_EXPR_REWRITES -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 05 Oct 2021 19:42:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 3: (1 comment) Hi Bikram, thank you for your review. Unfortunately, I just found one big problem in my patch. http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@108 PS3, Line 108: const TQueryExecRequest& query_request = request.query_exec_request; > nit: might be worth adding context here about the kind of plan created for This one seems to be a bigger obstacle for benchmarking expression. I take a closer looks and it seems the Planner rewrite most of constant query in this benchmark into literal expression through FoldConstantsRule https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java For example, "select 1+1;" will be rewritten into "select 2;", and "select from_unixtime(0, '-MM-dd');" will be rewritten into "select '1970-01-01';". I'll look around if we can disable this rewrite rule just for this benchmarking purpose. Otherwise, the benchmark does not make sense in the current state. -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 05 Oct 2021 18:36:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/17894 )
Change subject: IMPALA-10950: Update expr-benchmark.cc
..
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG@9
PS3, Line 9: Query planner has move the scalar expression's thrift definition
from
: 'fragments[0].output_sink.output_exprs' to
: 'fragments[0].plan.nodes[0].union_node.const_expr_lists[0]'.
which change made this switch? curious as to what the context there was
http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG@16
PS3, Line 16: some expressions
: in the benchmark now will crash without codegen due to missing
symbols
why are they now crashing if the benchmark earlier used to run fine without
codegen?
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc
File be/src/benchmarks/expr-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@108
PS3, Line 108: const TQueryExecRequest& query_request =
request.query_exec_request;
nit: might be worth adding context here about the kind of plan created for a
constant query
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@119
PS3, Line 119: // Uncomment the following lines for debugging.
: // union_node.printTo(cout);
: // cout << endl << " Need Codegen: " <<
test->fragment_state->ScalarExprNeedsCodegen()
: // << endl << endl;
do these need to be removed?
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@134
PS3, Line 134: codegen->EnableOptimizations(false);
why are we disabling this?
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@141
PS3, Line 141: void ReleaseTestData() {
: test_data_.clear();
: mem_pool_.FreeAll();
: }
nit: you can probably just add this to the destructor instead of a separate
function
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@183
PS3, Line 183: #define BENCHMARK(name, stmt) \
: suite->AddBenchmark(name, BenchmarkQueryFn,
GenerateBenchmarkExprs(stmt, true))
we should probably do both with and without codegen and also keep the benchmark
results for both, will be good to have historical data.
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@234
PS3, Line 234: // Cast: FunctionRate
Comparison
: //
--
: // int_to_int 824
1X
: //int_to_bool 878
1.066X
: // int_to_double 775.4
0.941X
: // int_to_string 32.47
0.03941X
: // double_to_boolean 823.5
0.9994X
: // double_to_bigint 775.4
0.941X
: // double_to_string 4.682
0.005682X
: // string_to_int 402.6
0.4886X
: //string_to_float 145.8
0.1769X
: //string_to_timestamp 83.76
0.1017X
all of these values are for benchmark without codegen, can you update these too?
--
To view, visit http://gerrit.cloudera.org:8080/17894
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f
Gerrit-Change-Number: 17894
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Tue, 05 Oct 2021 07:01:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/9549/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Sat, 02 Oct 2021 00:11:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Hello Gabor Kaszab, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17894 to look at the new patch set (#3). Change subject: IMPALA-10950: Update expr-benchmark.cc .. IMPALA-10950: Update expr-benchmark.cc Query planner has move the scalar expression's thrift definition from 'fragments[0].output_sink.output_exprs' to 'fragments[0].plan.nodes[0].union_node.const_expr_lists[0]'. This patch adjust expr-benchmark.cc to generate the ScalarExpr from the right thrift location and also modify the helper class to ensure proper resource cleanup at the end of benchmark. The benchmark used to be run without codegen. However, some expressions in the benchmark now will crash without codegen due to missing symbols. Those expressions include cast to decimal, asin, acos, and parse_url for PROTOCOL. Therefore, this patch enables codegen for all expressions being benchmarked. Testing: - Run and verify that expr-benchmark does not crash. Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f --- M be/src/benchmarks/expr-benchmark.cc 1 file changed, 138 insertions(+), 107 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/17894/3 -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 2: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/9548/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 01 Oct 2021 22:40:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Hello Gabor Kaszab, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17894 to look at the new patch set (#2). Change subject: IMPALA-10950: Update expr-benchmark.cc .. IMPALA-10950: Update expr-benchmark.cc Query planner has move the scalar expression's thrift definition from 'fragments[0].output_sink.output_exprs' to 'fragments[0].plan.nodes[0].union_node.const_expr_lists[0]'. This patch adjust expr-benchmark.cc to generate the ScalarExpr from the right thrift location and also modify the helper class to ensure proper resource cleanup at the end of benchmark. The benchmark used to be run without codegen. However, some expressions in the benchmark now will crash without codegen due to missing symbols. Those expressions include cast to decimal, asin, acos, and parse_url for PROTOCOL. Therefore, this patch enables codegen for all expressions being benchmarked. Testing: - Run and verify that expr-benchmark does not crash. Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f --- M be/src/benchmarks/expr-benchmark.cc 1 file changed, 134 insertions(+), 107 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/17894/2 -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/9547/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 01 Oct 2021 22:06:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17894 Change subject: IMPALA-10950: Update expr-benchmark.cc .. IMPALA-10950: Update expr-benchmark.cc Query planner has move the scalar expression's thrift definition from 'fragments[0].output_sink.output_exprs' to 'fragments[0].plan.nodes[0].union_node.const_expr_lists[0]'. This patch adjust expr-benchmark.cc to generate the ScalarExpr from the right thrift location and also modify the helper class to ensure proper resource cleanup at the end of benchmark. The benchmark used to be run without codegen. However, some expressions in the benchmark now will crash without codegen due to missing symbols. Those expressions include cast to decimal, asin, acos, and parse_url for PROTOCOL. Therefore, this patch enables codegen for all expressions being benchmarked. Testing: - Run and verify that expr-benchmark does not crash. Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f --- M be/src/benchmarks/expr-benchmark.cc 1 file changed, 128 insertions(+), 85 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/17894/1 -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto
