You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Riza Suminto (Code Review)" <ge...@cloudera.org> on 2021/10/01 21:42:53 UTC
[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 <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 02 Oct 2021 00:11:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Oct 2021 20:16:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Oct 2021 23:46:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 09 Oct 2021 02:23:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Oct 2021 22:06:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Oct 2021 05:39:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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, 'yyyy-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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Oct 2021 18:36:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Oct 2021 19:42:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Oct 2021 21:44:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Oct 2021 01:48:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Oct 2021 15:07:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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: Function Rate 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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Oct 2021 07:01:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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<TExpr> 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<unique_ptr<TestData>> 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<int64_t>(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)
: // ---------------------------------------------------------------------------------------------------------
: // equals 254 255 257 1X 1X 1X
: // not equals 320 322 324 1.26X 1.26X 1.26X
: // strstr 106 107 107 0.419X 0.419X 0.418X
: // strncmp1 215 216 217 0.848X 0.847X 0.845X
: // strncmp2 209 211 211 0.825X 0.825X 0.822X
: // strncmp3 254 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/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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Oct 2021 06:07:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Oct 2021 22:40:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Oct 2021 20:16:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Oct 2021 01:35:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Oct 2021 21:41:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Oct 2021 15:06:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Oct 2021 18:16:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Oct 2021 20:13:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
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 <ri...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>