[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins

2018-07-24 Thread Thomas Marshall (Code Review) 
Thomas Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11007 )

Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11007/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11007/4//COMMIT_MSG@7
PS4, Line 7: joins
Does this patch address joins?


http://gerrit.cloudera.org:8080/#/c/11007/4/be/src/runtime/buffered-tuple-stream-test.cc
File be/src/runtime/buffered-tuple-stream-test.cc:

http://gerrit.cloudera.org:8080/#/c/11007/4/be/src/runtime/buffered-tuple-stream-test.cc@678
PS4, Line 678: // Note: this should really be 0, but the BufferedTupleStream 
returns eos before
 :   // attaching the last buffer, rather than after, so the last 
block isn't deleted
 :   // until the stream is closed.
Is this comment still accurate? It is 0 now



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5
Gerrit-Change-Number: 11007
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 24 Jul 2018 22:53:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins

2018-07-24 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11007 )

Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins
..


Patch Set 4:

Build Successful

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5
Gerrit-Change-Number: 11007
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 24 Jul 2018 17:36:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins

2018-07-24 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11007 )

Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins
..


Patch Set 4:

Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/28/

Running initial code review checks. This is experimental - please report any 
issues to tarmstr...@cloudera.com or on this JIRA: IMPALA-7317


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5
Gerrit-Change-Number: 11007
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 24 Jul 2018 16:50:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins

2018-07-24 Thread Tim Armstrong (Code Review) 
Hello Thomas Marshall, Csaba Ringhofer, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins
..

IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins

This takes advantage of work (e.g. IMPALA-3200, IMPALA-5844)
to remove a couple of uses of the API.

Testing:
Ran core, ASAN and exhaustive builds.

Added a unit test to directly test the attaching behaviour.

Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5
---
M be/src/exec/grouping-aggregator.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/buffered-tuple-stream.cc
M be/src/runtime/buffered-tuple-stream.h
M be/src/runtime/row-batch.h
5 files changed, 235 insertions(+), 120 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5
Gerrit-Change-Number: 11007
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Marshall

[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins

2018-07-24 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11007 )

Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream-test.cc
File be/src/runtime/buffered-tuple-stream-test.cc:

http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream-test.cc@801
PS3, Line 801: true
> Shouldn't we pass attach_on_read here?
Yeah. It looks like I pushed out a stale version of the patchset missing some 
changes I made to this test.


http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream-test.cc@805
PS3, Line 805: ASSERT_OK(stream.GetNext(out_batch, ));
> Can you add an assert to check that out_batch->num_buffers() == 0 before Ge
Done. It is guaranteed by the out_batch->Reset() at the bottom of the loop but 
seems worth asserting.


http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream.h
File be/src/runtime/buffered-tuple-stream.h:

http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream.h@184
PS3, Line 184: /// caller is responsible for managing the lifetime of those 
buffers.
 : /// 3.  If the stream is unpinned and not in attach on read 
mode, then the batch returned
 : /// fro
> nit: is there a reason behind indenting these line with +1 space?
Nope, I'll make it consistent with above.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5
Gerrit-Change-Number: 11007
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 24 Jul 2018 16:50:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins

2018-07-24 Thread Csaba Ringhofer (Code Review) 
Csaba Ringhofer has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11007 )

Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream-test.cc
File be/src/runtime/buffered-tuple-stream-test.cc:

http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream-test.cc@801
PS3, Line 801: true
Shouldn't we pass attach_on_read here?


http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream-test.cc@805
PS3, Line 805: ASSERT_OK(stream.GetNext(out_batch, ));
Can you add an assert to check that out_batch->num_buffers() == 0 before 
GetNext()?


http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream.h
File be/src/runtime/buffered-tuple-stream.h:

http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream.h@184
PS3, Line 184: /// caller is responsible for managing the lifetime of those 
buffers.
 : /// 3.  If the stream is unpinned and not in attach on read 
mode, then the batch returned
 : /// fro
nit: is there a reason behind indenting these line with +1 space?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5
Gerrit-Change-Number: 11007
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Comment-Date: Tue, 24 Jul 2018 13:57:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins

2018-07-23 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11007 )

Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins
..


Patch Set 3:

Build Successful

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5
Gerrit-Change-Number: 11007
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Comment-Date: Tue, 24 Jul 2018 01:07:30 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins

2018-07-23 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11007 )

Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins
..


Patch Set 3:

Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/26/

Running initial code review checks. This is experimental - please report any 
issues to tarmstr...@cloudera.com or on this JIRA: IMPALA-7317


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5
Gerrit-Change-Number: 11007
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Comment-Date: Tue, 24 Jul 2018 00:44:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins

2018-07-23 Thread Tim Armstrong (Code Review) 
Hello Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins
..

IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins

This takes advantage of work (e.g. IMPALA-3200, IMPALA-5844)
to remove a couple of uses of the API.

Testing:
Ran core, ASAN and exhaustive builds.

Added a unit test to directly test the attaching behaviour.

Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5
---
M be/src/exec/grouping-aggregator.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/buffered-tuple-stream.cc
M be/src/runtime/buffered-tuple-stream.h
4 files changed, 228 insertions(+), 120 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/11007/3
--
To view, visit http://gerrit.cloudera.org:8080/11007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5
Gerrit-Change-Number: 11007
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins