You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2018/08/24 17:00:07 UTC

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction 

Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11297


Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................

IMPALA-7477: Batch-oriented query set construction

Rework the row-by-row construction of query result sets in PlanRootSink
so that it materialises an output column at a time. Make some minor
optimisations like preallocating output vectors and initialising
strings more efficiently.

My intent is both to make this faster and to make the QueryResultSet
interface better before IMPALA-4268 does a bunch of surgery on this
part of the code.

Testing:
Ran core tests.

Perf:
Downloaded tpch_parquet.orders via JDBC driver.
Before: 3.01s, After: 2.57s.

Downloaded l_orderkey from tpch_parquet.lineitem.
Before: 1.21s, After: 1.08s.

Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
---
M be/src/exec/plan-root-sink.cc
M be/src/exec/plan-root-sink.h
M be/src/service/hs2-util.cc
M be/src/service/hs2-util.h
M be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
6 files changed, 305 insertions(+), 158 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/11297/4
-- 
To view, visit http://gerrit.cloudera.org:8080/11297
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

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/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................

IMPALA-7477: Batch-oriented query set construction

Rework the row-by-row construction of query result sets in PlanRootSink
so that it materialises an output column at a time. Make some minor
optimisations like preallocating output vectors and initialising
strings more efficiently.

My intent is both to make this faster and to make the QueryResultSet
interface better before IMPALA-4268 does a bunch of surgery on this
part of the code.

Testing:
Ran core tests.

Perf:
Downloaded tpch_parquet.orders via JDBC driver.
Before: 3.01s, After: 2.57s.

Downloaded l_orderkey from tpch_parquet.lineitem.
Before: 1.21s, After: 1.08s.

Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Reviewed-on: http://gerrit.cloudera.org:8080/11297
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/plan-root-sink.cc
M be/src/exec/plan-root-sink.h
M be/src/service/hs2-util.cc
M be/src/service/hs2-util.h
M be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
6 files changed, 320 insertions(+), 161 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 10
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 8: Code-Review+1

carry


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Aug 2018 15:52:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 7:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3089/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Aug 2018 23:24:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3109/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 9
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Sep 2018 13:57:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Borok-Nagy, Todd Lipcon, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................

IMPALA-7477: Batch-oriented query set construction

Rework the row-by-row construction of query result sets in PlanRootSink
so that it materialises an output column at a time. Make some minor
optimisations like preallocating output vectors and initialising
strings more efficiently.

My intent is both to make this faster and to make the QueryResultSet
interface better before IMPALA-4268 does a bunch of surgery on this
part of the code.

Testing:
Ran core tests.

Perf:
Downloaded tpch_parquet.orders via JDBC driver.
Before: 3.01s, After: 2.57s.

Downloaded l_orderkey from tpch_parquet.lineitem.
Before: 1.21s, After: 1.08s.

Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
---
M be/src/exec/plan-root-sink.cc
M be/src/exec/plan-root-sink.h
M be/src/service/hs2-util.cc
M be/src/service/hs2-util.h
M be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
6 files changed, 320 insertions(+), 161 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/11297/8
-- 
To view, visit http://gerrit.cloudera.org:8080/11297
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/518/ : 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/11297
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Aug 2018 16:36:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/482/ : 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/11297
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 25 Aug 2018 01:27:43 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 9: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 9
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Sep 2018 17:11:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Borok-Nagy, Todd Lipcon, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................

IMPALA-7477: Batch-oriented query set construction

Rework the row-by-row construction of query result sets in PlanRootSink
so that it materialises an output column at a time. Make some minor
optimisations like preallocating output vectors and initialising
strings more efficiently.

My intent is both to make this faster and to make the QueryResultSet
interface better before IMPALA-4268 does a bunch of surgery on this
part of the code.

Testing:
Ran core tests.

Perf:
Downloaded tpch_parquet.orders via JDBC driver.
Before: 3.01s, After: 2.57s.

Downloaded l_orderkey from tpch_parquet.lineitem.
Before: 1.21s, After: 1.08s.

Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
---
M be/src/exec/plan-root-sink.cc
M be/src/exec/plan-root-sink.h
M be/src/service/hs2-util.cc
M be/src/service/hs2-util.h
M be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
6 files changed, 320 insertions(+), 161 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/11297/6
-- 
To view, visit http://gerrit.cloudera.org:8080/11297
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Borok-Nagy, Todd Lipcon, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................

IMPALA-7477: Batch-oriented query set construction

Rework the row-by-row construction of query result sets in PlanRootSink
so that it materialises an output column at a time. Make some minor
optimisations like preallocating output vectors and initialising
strings more efficiently.

My intent is both to make this faster and to make the QueryResultSet
interface better before IMPALA-4268 does a bunch of surgery on this
part of the code.

Testing:
Ran core tests.

Perf:
Downloaded tpch_parquet.orders via JDBC driver.
Before: 3.01s, After: 2.57s.

Downloaded l_orderkey from tpch_parquet.lineitem.
Before: 1.21s, After: 1.08s.

Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
---
M be/src/exec/plan-root-sink.cc
M be/src/exec/plan-root-sink.h
M be/src/service/hs2-util.cc
M be/src/service/hs2-util.h
M be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
6 files changed, 314 insertions(+), 159 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/11297/5
-- 
To view, visit http://gerrit.cloudera.org:8080/11297
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/471/ : 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/11297
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 24 Aug 2018 17:33:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/11297/5/be/src/service/query-result-set.cc
File be/src/service/query-result-set.cc:

http://gerrit.cloudera.org:8080/#/c/11297/5/be/src/service/query-result-set.cc@87
PS5, Line 87: /// Add a row of expr values
nit: Please update comment


http://gerrit.cloudera.org:8080/#/c/11297/5/be/src/service/query-result-set.cc@179
PS5, Line 179: num_rows
num_rows - start_idx ?


http://gerrit.cloudera.org:8080/#/c/11297/5/be/src/service/query-result-set.cc@285
PS5, Line 285: // Add a row of expr values
nit: this comment is also obsolete


http://gerrit.cloudera.org:8080/#/c/11297/5/be/src/service/query-result-set.cc@456
PS5, Line 456:   DCHECK_EQ(num_col, metadata_.columns.size());
Maybe we could use reserve() on result_set_ here as well.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Aug 2018 14:32:10 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/11297/5/be/src/service/query-result-set.cc
File be/src/service/query-result-set.cc:

http://gerrit.cloudera.org:8080/#/c/11297/5/be/src/service/query-result-set.cc@57
PS5, Line 57:   /// Convert rows expr values stored in 'col_values' to ASCII using "\t" as column
This needed updating too.


http://gerrit.cloudera.org:8080/#/c/11297/5/be/src/service/query-result-set.cc@87
PS5, Line 87: /// Add a row of expr values
> nit: Please update comment
Done


http://gerrit.cloudera.org:8080/#/c/11297/5/be/src/service/query-result-set.cc@179
PS5, Line 179: num_rows
> num_rows - start_idx ?
Done


http://gerrit.cloudera.org:8080/#/c/11297/5/be/src/service/query-result-set.cc@285
PS5, Line 285: // Add a row of expr values
> nit: this comment is also obsolete
Done


http://gerrit.cloudera.org:8080/#/c/11297/5/be/src/service/query-result-set.cc@456
PS5, Line 456:   DCHECK_EQ(num_col, metadata_.columns.size());
> Maybe we could use reserve() on result_set_ here as well.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Aug 2018 23:23:58 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/511/ : 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/11297
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Aug 2018 23:55:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 8: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Sep 2018 13:56:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11297/4/be/src/service/hs2-util.cc
File be/src/service/hs2-util.cc:

http://gerrit.cloudera.org:8080/#/c/11297/4/be/src/service/hs2-util.cc@128
PS4, Line 128: ReserveSpace
Just passing through here... is it possible that this reserving design will end up with an "accidentally quadratic" scenario in the case that rows are added a few at a time? I'm not sure at what level RowBatches get aggregated together into batches that get delivered to the client, but if you had a RowBatch size of 1000 rows, and some selective predicate such that each RowBatch only has one or two matches, would we end up calling AddRows() with num_rows=1 in a loop anywhere?

If that's the case we may want to do something here like reserve the next larger power-of-two capacity rather than "size to fit this next batch"



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 24 Aug 2018 22:58:39 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 7: Code-Review+1

(1 comment)

Found a small issue.
After fixing it I can +2 it. Unless you want others to look at it too.

http://gerrit.cloudera.org:8080/#/c/11297/7/be/src/service/hs2-util.cc
File be/src/service/hs2-util.cc:

http://gerrit.cloudera.org:8080/#/c/11297/7/be/src/service/hs2-util.cc@137
PS7, Line 137: values
I think it should be 'nulls'.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Aug 2018 14:19:52 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 7: Code-Review+1

didn't look in detail but the round-up-to-power-of-two thing looks good to me


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Aug 2018 01:07:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11297/7/be/src/service/hs2-util.cc
File be/src/service/hs2-util.cc:

http://gerrit.cloudera.org:8080/#/c/11297/7/be/src/service/hs2-util.cc@137
PS7, Line 137: values
> I think it should be 'nulls'.
Thanks for catching



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Aug 2018 15:52:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 7: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Aug 2018 02:50:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7477: Batch-oriented query set construction

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11297 )

Change subject: IMPALA-7477: Batch-oriented query set construction
......................................................................


Patch Set 9: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc87a84c34935d0d5841c7f5528eb802527fa809
Gerrit-Change-Number: 11297
Gerrit-PatchSet: 9
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Sep 2018 13:57:17 +0000
Gerrit-HasComments: No