Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15963 )

Change subject: IMPALA-6692: Trigger sort node run before hitting memory limit.
......................................................................


Patch Set 12:

(3 comments)

Thank you Csaba for your feedback!
I have couple follow up questions.

http://gerrit.cloudera.org:8080/#/c/15963/12/be/src/exec/sort-node.cc
File be/src/exec/sort-node.cc:

http://gerrit.cloudera.org:8080/#/c/15963/12/be/src/exec/sort-node.cc@89
PS12, Line 89: cardinality
> What is the default value of this? Can it be -1 (unknown)? The result seems
Ok, in that case, we should just abandon estimate in case of cardinality is -1.


http://gerrit.cloudera.org:8080/#/c/15963/12/be/src/exec/sort-node.cc@90
PS12, Line 90: GetRowSize
> I think that this doesn't contain varlen data, so it can greatly underestim
So what is the nature of varlen column? Is each row possibly will have 
different sizes with large variations?
And what is GetRowSize() return in that case? Thinking if we should abandon the 
estimate entirely for input rows having varlen data.


http://gerrit.cloudera.org:8080/#/c/15963/12/tests/query_test/test_sort.py
File tests/query_test/test_sort.py:

http://gerrit.cloudera.org:8080/#/c/15963/12/tests/query_test/test_sort.py@74
PS12, Line 74:     """The first sort run is given a privilege to ignore 
sort_run_bytes_limit, except
             :        when estimate hints that spill is inevitable. The lower 
sort_run_bytes_limit of
             :        a query is, the more sort runs are likely to be produced.
             :        Case 1 : 1 run produced, because all rows fit within the 
maximum reservation.
             :                 sort_run_bytes_limit is not enforced.
             :        Case 2 : 3 run produced, because the first run hit 
reservation limit, and the
             :                 next 2 runs are capped to 150m.
             :        Case 3 : 4 run produced, because sort node estimate that 
spill is inevitable.
             :                 So all runs are capped to 130m, including the 
first one."""
> Isn't there something in query_result.runtime_profile that could be used to
I will look at that 'query_result.runtime_profile'.
Otherwise, I will change this test to run_test_case and verify the profile via 
regex.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a0ba7c4bae4f1d300d4d9d7f594f63ced06a240
Gerrit-Change-Number: 15963
Gerrit-PatchSet: 12
Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com>
Gerrit-Reviewer: David Rorke <dro...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Jun 2020 16:15:34 +0000
Gerrit-HasComments: Yes

Reply via email to