You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2018/04/12 03:43:06 UTC

[Impala-ASF-CR] IMPALA-6713: Fix request for unneeded memory in partial sort 

Thomas Tauber-Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10031


Change subject: IMPALA-6713: Fix request for unneeded memory in partial sort
......................................................................

IMPALA-6713: Fix request for unneeded memory in partial sort

When a Sorter::Run is initialzed, if it is an initial run and has
varlen data, it requests an extra buffer to have space to sort the
varlen data should it need to spill to disk.

This extra buffer is not needed in the case of partial sorts, which
do not spill, and because this extra buffer was not included in the
calculation of the minimum required reservation, requesting it caused
the partial sort to fail in cases where the partial sort only had its
minimum reservation available to use.

The solution is to not request the extra memory for partial sorts.

Testing:
- Added a test to test_sort.py that ensures the partial sort can
  complete successfully even if additional memory requests beyond its
  minimum reservation are denied.

Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
---
M be/src/runtime/sorter.cc
M tests/query_test/test_sort.py
2 files changed, 19 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/10031/1
-- 
To view, visit http://gerrit.cloudera.org:8080/10031
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Gerrit-Change-Number: 10031
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>

[Impala-ASF-CR] IMPALA-6713: Fix request for unneeded memory in partial sort

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/10031 )

Change subject: IMPALA-6713: Fix request for unneeded memory in partial sort
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10031/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10031/1//COMMIT_MSG@9
PS1, Line 9: initialize
> nit: typo
Done


http://gerrit.cloudera.org:8080/#/c/10031/1/be/src/runtime/sorter.cc
File be/src/runtime/sorter.cc:

http://gerrit.cloudera.org:8080/#/c/10031/1/be/src/runtime/sorter.cc@634
PS1, Line 634:       + (has_var_len_slots_ && initial_run_ && sorter_->enable_spilling_);
> Should we also test that a regular Sort node with spilling turned off works
I think this case is already covered by various tests is test_spilling.py



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Gerrit-Change-Number: 10031
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 19:34:01 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6713: Fix request for unneeded memory in partial sort

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

Change subject: IMPALA-6713: Fix request for unneeded memory in partial sort
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10031/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10031/1//COMMIT_MSG@9
PS1, Line 9: initialzed
nit: typo


http://gerrit.cloudera.org:8080/#/c/10031/1/be/src/runtime/sorter.cc
File be/src/runtime/sorter.cc:

http://gerrit.cloudera.org:8080/#/c/10031/1/be/src/runtime/sorter.cc@634
PS1, Line 634:       + (has_var_len_slots_ && initial_run_ && sorter_->enable_spilling_);
Should we also test that a regular Sort node with spilling turned off works well with the lowest memory allocation that was needed before this change? (e.g. picking any sorting query checking what the lowest memory limit it worked with spilling turned off, check that it still works with that limit)
There might be an existing test for this, though.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Gerrit-Change-Number: 10031
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 18:25:13 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6713: Fix request for unneeded memory in partial sort

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

Change subject: IMPALA-6713: Fix request for unneeded memory in partial sort
......................................................................


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Gerrit-Change-Number: 10031
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 06:54:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6713: Fix request for unneeded memory in partial sort

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

Change subject: IMPALA-6713: Fix request for unneeded memory in partial sort
......................................................................


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Gerrit-Change-Number: 10031
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 21:20:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6713: Fix request for unneeded memory in partial sort

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

Change subject: IMPALA-6713: Fix request for unneeded memory in partial sort
......................................................................

IMPALA-6713: Fix request for unneeded memory in partial sort

When a Sorter::Run is initialized, if it is an initial run and has
varlen data, it requests an extra buffer to have space to sort the
varlen data should it need to spill to disk.

This extra buffer is not needed in the case of partial sorts, which
do not spill, and because this extra buffer was not included in the
calculation of the minimum required reservation, requesting it caused
the partial sort to fail in cases where the partial sort only had its
minimum reservation available to use.

The solution is to not request the extra memory for partial sorts.

Testing:
- Added a test to test_sort.py that ensures the partial sort can
  complete successfully even if additional memory requests beyond its
  minimum reservation are denied.

Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Reviewed-on: http://gerrit.cloudera.org:8080/10031
Reviewed-by: Thomas Tauber-Marshall <tm...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/sorter.cc
M tests/query_test/test_sort.py
2 files changed, 19 insertions(+), 2 deletions(-)

Approvals:
  Thomas Tauber-Marshall: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Gerrit-Change-Number: 10031
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-6713: Fix request for unneeded memory in partial sort

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

Change subject: IMPALA-6713: Fix request for unneeded memory in partial sort
......................................................................


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2306/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Gerrit-Change-Number: 10031
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 17:32:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6713: Fix request for unneeded memory in partial sort

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

Change subject: IMPALA-6713: Fix request for unneeded memory in partial sort
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

LGTM so long as we're not running the test with unnecessary dimensions.

http://gerrit.cloudera.org:8080/#/c/10031/2/tests/query_test/test_sort.py
File tests/query_test/test_sort.py:

http://gerrit.cloudera.org:8080/#/c/10031/2/tests/query_test/test_sort.py@209
PS2, Line 209:   """Test class to do functional validation of partial sorts."""
I forget what the default test matrix is - does this only run with a single table format?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Gerrit-Change-Number: 10031
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 16:02:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6713: Fix request for unneeded memory in partial sort

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/10031 )

Change subject: IMPALA-6713: Fix request for unneeded memory in partial sort
......................................................................


Patch Set 3: Code-Review+2

carrying forward


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Gerrit-Change-Number: 10031
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 17:32:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6713: Fix request for unneeded memory in partial sort

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Gabor Kaszab, Tim Armstrong, 

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

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

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

Change subject: IMPALA-6713: Fix request for unneeded memory in partial sort
......................................................................

IMPALA-6713: Fix request for unneeded memory in partial sort

When a Sorter::Run is initialized, if it is an initial run and has
varlen data, it requests an extra buffer to have space to sort the
varlen data should it need to spill to disk.

This extra buffer is not needed in the case of partial sorts, which
do not spill, and because this extra buffer was not included in the
calculation of the minimum required reservation, requesting it caused
the partial sort to fail in cases where the partial sort only had its
minimum reservation available to use.

The solution is to not request the extra memory for partial sorts.

Testing:
- Added a test to test_sort.py that ensures the partial sort can
  complete successfully even if additional memory requests beyond its
  minimum reservation are denied.

Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
---
M be/src/runtime/sorter.cc
M tests/query_test/test_sort.py
2 files changed, 19 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/10031/2
-- 
To view, visit http://gerrit.cloudera.org:8080/10031
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Gerrit-Change-Number: 10031
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-6713: Fix request for unneeded memory in partial sort

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/10031 )

Change subject: IMPALA-6713: Fix request for unneeded memory in partial sort
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10031/2/tests/query_test/test_sort.py
File tests/query_test/test_sort.py:

http://gerrit.cloudera.org:8080/#/c/10031/2/tests/query_test/test_sort.py@209
PS2, Line 209:   """Test class to do functional validation of partial sorts."""
> I forget what the default test matrix is - does this only run with a single
Tests that don't take 'vector' as a parameter are just run a single time without any dimensions



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d9c0863009021340d8b684669b371a2cfb1ecad
Gerrit-Change-Number: 10031
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 17:27:27 +0000
Gerrit-HasComments: Yes