You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Noemi Pap-Takacs (Code Review)" <ge...@cloudera.org> on 2024/09/10 13:15:35 UTC

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

Noemi Pap-Takacs has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21777


Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................

IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0

test_sort.py declared 'max_sort_run_size' query option, but it
silently did not exercise it. Fixing the query option declaration
in IMPALA-13349 using helper function add_exec_option_dimension()
revealed a DCHECK failure in sorter.cc. In some cases the length
of an in-memory run could exceed 'max_sort_run_size' by 1 page.

This patch fixed the DCHECK failure by strictly enforcing the
'max_sort_run_size' limit.
Memory limits were also adjusted in test_sort.py according to
the memory usage of the different sort implementations.

Testing:
- E2E tests in sort.py
- SET test in set.py

Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
---
M be/src/runtime/sorter.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
M testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test
M tests/query_test/test_sort.py
4 files changed, 56 insertions(+), 21 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 1
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 8:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 8
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Nov 2024 16:13:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 8: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21777/8/tests/query_test/test_sort.py
File tests/query_test/test_sort.py:

http://gerrit.cloudera.org:8080/#/c/21777/8/tests/query_test/test_sort.py@404
PS8, Line 404:     # Check that spilling was successful.
             :     assert re.search(r'\s+\- SpilledRuns: [1-9]', query_result.runtime_profile)
nit: This is OK if num SpilledRuns does not need to be consistent between test_array_sort and test_array_sort_with_limit. If it is indeed important, then this assertion might be better:

    if exec_option['max_sort_run_size'] > 0:
      assert re.search(r'\s+\- SpilledRuns: [1-9]', query_result.runtime_profile)
    else:
      assert "SpilledRuns: 3" in query_result.runtime_profile

Please double check with Daniel.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 8
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Nov 2024 21:50:39 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 8: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 8
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Nov 2024 09:41:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21777/2/testdata/workloads/functional-query/queries/QueryTest/set.test
File testdata/workloads/functional-query/queries/QueryTest/set.test:

http://gerrit.cloudera.org:8080/#/c/21777/2/testdata/workloads/functional-query/queries/QueryTest/set.test@308
PS2, Line 308: ---- QUERY
Can you add similar test in query-options-test.cc (QueryOptions.SetIntOptions)?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 2
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Sep 2024 14:42:46 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................

IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0

test_sort.py declared 'max_sort_run_size' query option, but it
silently did not exercise it. Fixing the query option declaration
in IMPALA-13349 using helper function add_exec_option_dimension()
revealed a DCHECK failure in sorter.cc. In some cases the length
of an in-memory run could exceed 'max_sort_run_size' by 1 page.

This patch fixed the DCHECK failure by strictly enforcing the
'max_sort_run_size' limit.
Memory limits were also adjusted in test_sort.py according to
the memory usage of the different sort run sizes.

Additionally, the 'MAX_SORT_RUN_SIZE' query option's valid range was
relaxed. Instead of throwing an error, negative values also disable
the run size limitation, just as the default: '0'.

Testing:
- E2E tests in sort.py
- set test

Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Reviewed-on: http://gerrit.cloudera.org:8080/21777
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/sorter-internal.h
M be/src/runtime/sorter.cc
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M common/thrift/ImpalaService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test
M tests/query_test/test_sort.py
8 files changed, 91 insertions(+), 38 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 10
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 6: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21777/6/tests/query_test/test_sort.py
File tests/query_test/test_sort.py:

http://gerrit.cloudera.org:8080/#/c/21777/6/tests/query_test/test_sort.py@115
PS6, Line 115:     options = [('2g', '100m', '0'), ('400m', '130m', '5')]
> I'd suggest to rewrite into this for clarity:
Out of these two, I'd take the second approach, because if we're redefining the whole 'options' list, it's more difficult to spot where the difference is.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 6
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Oct 2024 13:34:44 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

Posted by "Noemi Pap-Takacs (Code Review)" <ge...@cloudera.org>.
Noemi Pap-Takacs has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/21777 )

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................

IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0

test_sort.py declared 'max_sort_run_size' query option, but it
silently did not exercise it. Fixing the query option declaration
in IMPALA-13349 using helper function add_exec_option_dimension()
revealed a DCHECK failure in sorter.cc. In some cases the length
of an in-memory run could exceed 'max_sort_run_size' by 1 page.

This patch fixed the DCHECK failure by strictly enforcing the
'max_sort_run_size' limit.
Memory limits were also adjusted in test_sort.py according to
the memory usage of the different sort run sizes.

Additionally, the 'MAX_SORT_RUN_SIZE' query option's valid range was
relaxed. Instead of throwing an error, negative values also disable
the run size limitation, just as the default: '0'.

Testing:
- E2E tests in sort.py
- set test

Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
---
M be/src/runtime/sorter-internal.h
M be/src/runtime/sorter.cc
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M common/thrift/ImpalaService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test
M tests/query_test/test_sort.py
8 files changed, 91 insertions(+), 38 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 8
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 2:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 2
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Sep 2024 12:51:49 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

Posted by "Noemi Pap-Takacs (Code Review)" <ge...@cloudera.org>.
Noemi Pap-Takacs has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/21777 )

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................

IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0

test_sort.py declared 'max_sort_run_size' query option, but it
silently did not exercise it. Fixing the query option declaration
in IMPALA-13349 using helper function add_exec_option_dimension()
revealed a DCHECK failure in sorter.cc. In some cases the length
of an in-memory run could exceed 'max_sort_run_size' by 1 page.

This patch fixed the DCHECK failure by strictly enforcing the
'max_sort_run_size' limit.
Memory limits were also adjusted in test_sort.py according to
the memory usage of the different sort run sizes.

Testing:
- E2E tests in sort.py
- SET test in set.py
- backend test in query-options-test.cc

Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
---
M be/src/runtime/sorter-internal.h
M be/src/runtime/sorter.cc
M be/src/service/query-options-test.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
M testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test
M tests/query_test/test_sort.py
6 files changed, 71 insertions(+), 22 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 3
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 6:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 6
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 14:20:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 3:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 3
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2024 16:06:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 1:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 1
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Sep 2024 13:49:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 8: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 8
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Nov 2024 21:21:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 8: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21777/8/tests/query_test/test_sort.py
File tests/query_test/test_sort.py:

http://gerrit.cloudera.org:8080/#/c/21777/8/tests/query_test/test_sort.py@404
PS8, Line 404:     # Check that spilling was successful.
             :     assert re.search(r'\s+\- SpilledRuns: [1-9]', query_result.runtime_profile)
> nit: This is OK if num SpilledRuns does not need to be consistent between t
The exact number is not important, my intention was to test the spilling path.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 8
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Nov 2024 09:41:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21777/3/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

http://gerrit.cloudera.org:8080/#/c/21777/3/be/src/service/query-options-test.cc@274
PS3, Line 274: {MAKE_OPTIONDEF(max_sort_run_size),              {0, 0}},
             :       {MAKE_OPTIONDEF(max_sort_run_size),              {2, I32_MAX}},
> So your idea is that the entire range should be considered valid, but value
I agree that the value 1 should not be allowed. A user could reasonably think that sort runs of size 1 are okay (but they are actually not). If there is no error, it can lead to confusion that the user thinks the value is set to 1 when it is actually set to some other value.
Zero and negative values don't have this problem because it's not reasonable to think that they are valid valid values.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 3
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Sep 2024 14:34:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21777/1/testdata/workloads/functional-query/queries/QueryTest/set.test
File testdata/workloads/functional-query/queries/QueryTest/set.test:

http://gerrit.cloudera.org:8080/#/c/21777/1/testdata/workloads/functional-query/queries/QueryTest/set.test@316
PS1, Line 316: Invalid value for query option MAX_SORT_RUN_SIZE: Value must be non-negative: -1
> I think that this would be the scope of another change, since this error me
Ok.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 2
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Sep 2024 14:42:17 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

Posted by "Noemi Pap-Takacs (Code Review)" <ge...@cloudera.org>.
Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/21777 )

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 3:

(1 comment)

Thanks for the review.

http://gerrit.cloudera.org:8080/#/c/21777/2/testdata/workloads/functional-query/queries/QueryTest/set.test
File testdata/workloads/functional-query/queries/QueryTest/set.test:

http://gerrit.cloudera.org:8080/#/c/21777/2/testdata/workloads/functional-query/queries/QueryTest/set.test@308
PS2, Line 308: ---- QUERY
> Can you add similar test in query-options-test.cc (QueryOptions.SetIntOptio
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 3
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2024 15:35:36 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21777/3/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

http://gerrit.cloudera.org:8080/#/c/21777/3/be/src/service/query-options-test.cc@274
PS3, Line 274: {MAKE_OPTIONDEF(max_sort_run_size),              {0, 0}},
             :       {MAKE_OPTIONDEF(max_sort_run_size),              {2, I32_MAX}},
nit: just question, have you consider treating negative and 1 as disabling toggle just like 0, instead of throwing error?
That will simplify valid range for this option, right?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 3
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2024 16:00:13 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 3:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/21777/3/be/src/runtime/sorter.cc@281
PS3, Line 281: MaxSortRunSizeReached()
For performance reasons, you could write

 if (INITIAL_RUN && MaxSortRunSizeReached())

Or, even better, make MaxSortRunSizeReached take INITIAL_RUN as template parameter. AFAICT MaxSortRunSizeReached() is only invoked in context where we already have INITIAL_RUN template parameter:

 if (MaxSortRunSizeReached<INITIAL_RUN>())


http://gerrit.cloudera.org:8080/#/c/21777/3/be/src/runtime/sorter.cc@299
PS3, Line 299:  
you could add inline / IR_ALWAYS_INLINE here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 3
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Sep 2024 14:31:38 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 7:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 7
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 15:57:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 9:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 9
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Nov 2024 09:45:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 9: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 9
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Nov 2024 09:45:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 7:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 7
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sun, 03 Nov 2024 15:35:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 8:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 8
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Nov 2024 15:42:01 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 1:

(9 comments)

Thanks for the quick fix!

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

http://gerrit.cloudera.org:8080/#/c/21777/1//COMMIT_MSG@18
PS1, Line 18: different sort implementations
This means the different max sort run sizes, right? It would be clearer to say that.


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

http://gerrit.cloudera.org:8080/#/c/21777/1/be/src/runtime/sorter.cc@247
PS1, Line 247: INITIAL_RUN && max_num_of_pages_ > 0 && run_size() == max_num_of_pages_
This condition could be extracted to a function in order to give it a name that makes reading easier. It could also be used on L281.


http://gerrit.cloudera.org:8080/#/c/21777/1/be/src/runtime/sorter.cc@281
PS1, Line 281: ==
Was this changed from ">=" to "==" because run_size() is never greater than max_num_of_pages_? If so, could add a DCHECK (see also the comment on L247).


http://gerrit.cloudera.org:8080/#/c/21777/1/testdata/workloads/functional-query/queries/QueryTest/set.test
File testdata/workloads/functional-query/queries/QueryTest/set.test:

http://gerrit.cloudera.org:8080/#/c/21777/1/testdata/workloads/functional-query/queries/QueryTest/set.test@316
PS1, Line 316: Invalid value for query option MAX_SORT_RUN_SIZE: Value must be non-negative: -1
Adding "actual value", like on L311, would be clearer. I know it's not completely related to this patch, but if it's not hard to change, I think it's worth it.


http://gerrit.cloudera.org:8080/#/c/21777/1/tests/query_test/test_sort.py
File tests/query_test/test_sort.py:

http://gerrit.cloudera.org:8080/#/c/21777/1/tests/query_test/test_sort.py@31
PS1, Line 31: encounter
Nit: "encounters"


http://gerrit.cloudera.org:8080/#/c/21777/1/tests/query_test/test_sort.py@32
PS1, Line 32: smallest possible size of a run
Could add why 2 is the smallest valid value (fix-len and var-len pages).


http://gerrit.cloudera.org:8080/#/c/21777/1/tests/query_test/test_sort.py@35
PS1, Line 35: apply
Maybe "are needed to trigger the same scenarios" would be better?


http://gerrit.cloudera.org:8080/#/c/21777/1/tests/query_test/test_sort.py@168
PS1, Line 168:       exec_option['mem_limit'] = "144m"
This could be merged with L165:
  exec_option['mem_limit'] = "144m" if exec_option['max_sort_run_size'] == 2 else "134m"


http://gerrit.cloudera.org:8080/#/c/21777/1/tests/query_test/test_sort.py@244
PS1, Line 244:       self.run_test_case('sort-reservation-usage-single-node', vector,
Instead of calling this function in two conditional branches, you could extract the limit into a variable and use that as (part of) the parameter.

Could do something similar on L401 with "string.format()".



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 1
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Sep 2024 14:29:59 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

Posted by "Noemi Pap-Takacs (Code Review)" <ge...@cloudera.org>.
Noemi Pap-Takacs has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/21777 )

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................

IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0

test_sort.py declared 'max_sort_run_size' query option, but it
silently did not exercise it. Fixing the query option declaration
in IMPALA-13349 using helper function add_exec_option_dimension()
revealed a DCHECK failure in sorter.cc. In some cases the length
of an in-memory run could exceed 'max_sort_run_size' by 1 page.

This patch fixed the DCHECK failure by strictly enforcing the
'max_sort_run_size' limit.
Memory limits were also adjusted in test_sort.py according to
the memory usage of the different sort run sizes.

Testing:
- E2E tests in sort.py
- SET test in set.py

Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
---
M be/src/runtime/sorter-internal.h
M be/src/runtime/sorter.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
M testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test
M tests/query_test/test_sort.py
5 files changed, 69 insertions(+), 22 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 2
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 9: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 9
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Nov 2024 15:06:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 7: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11097/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 7
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sun, 03 Nov 2024 20:42:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

Posted by "Noemi Pap-Takacs (Code Review)" <ge...@cloudera.org>.
Noemi Pap-Takacs has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/21777 )

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................

IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0

test_sort.py declared 'max_sort_run_size' query option, but it
silently did not exercise it. Fixing the query option declaration
in IMPALA-13349 using helper function add_exec_option_dimension()
revealed a DCHECK failure in sorter.cc. In some cases the length
of an in-memory run could exceed 'max_sort_run_size' by 1 page.

This patch fixed the DCHECK failure by strictly enforcing the
'max_sort_run_size' limit.
Memory limits were also adjusted in test_sort.py according to
the memory usage of the different sort run sizes.

Additionally, the 'MAX_SORT_RUN_SIZE' query option's valid range was
relaxed. Instead of throwing an error, negative values also disable
the run size limitation, just as the default: '0'.

Testing:
- E2E tests in sort.py
- set test

Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
---
M be/src/runtime/sorter-internal.h
M be/src/runtime/sorter.cc
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M common/thrift/ImpalaService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test
M tests/query_test/test_sort.py
8 files changed, 95 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/21777/7
-- 
To view, visit http://gerrit.cloudera.org:8080/21777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 7
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 7: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 7
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 15:25:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 3: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 3
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2024 15:46:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

Posted by "Noemi Pap-Takacs (Code Review)" <ge...@cloudera.org>.
Noemi Pap-Takacs has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/21777 )

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................

IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0

test_sort.py declared 'max_sort_run_size' query option, but it
silently did not exercise it. Fixing the query option declaration
in IMPALA-13349 using helper function add_exec_option_dimension()
revealed a DCHECK failure in sorter.cc. In some cases the length
of an in-memory run could exceed 'max_sort_run_size' by 1 page.

This patch fixed the DCHECK failure by strictly enforcing the
'max_sort_run_size' limit.
Memory limits were also adjusted in test_sort.py according to
the memory usage of the different sort run sizes.

Additionally, the 'MAX_SORT_RUN_SIZE' query option's valid range was
relaxed. Instead of throwing an error, negative values also disable
the run size limitation, just as the default: '0'.

Testing:
- E2E tests in sort.py
- set test

Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
---
M be/src/runtime/sorter-internal.h
M be/src/runtime/sorter.cc
M be/src/service/query-options.cc
M common/thrift/ImpalaService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test
M tests/query_test/test_sort.py
7 files changed, 81 insertions(+), 37 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 6
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-13334: Fix test sort.py DCHECK hit when max sort run size>0

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

Change subject: IMPALA-13334: Fix test_sort.py DCHECK hit when max_sort_run_size>0
......................................................................


Patch Set 6:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/21777/3/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

http://gerrit.cloudera.org:8080/#/c/21777/3/be/src/service/query-options-test.cc@274
PS3, Line 274: 
             :   for (const auto& test_case : case_set) {
> Ok, I can accept Daniels argument.
I think this test can stay. I notice this is gone in ps6 in favor of set.test.
I also suggest moving this to under SetSpecialOptions test to add max_sort_run_size=1 testcase.


http://gerrit.cloudera.org:8080/#/c/21777/6/testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test
File testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test:

http://gerrit.cloudera.org:8080/#/c/21777/6/testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test@9
PS6, Line 9: 2m
"2MB" to differentiate against 2 millions.


http://gerrit.cloudera.org:8080/#/c/21777/5/tests/query_test/test_sort.py
File tests/query_test/test_sort.py:

http://gerrit.cloudera.org:8080/#/c/21777/5/tests/query_test/test_sort.py@168
PS5, Line 168:     if exec_option['max_sort_run_size'] == 2:
             :       exec_option['mem_limit'] = "144m"
> AFAIK if memory estimation cannot be calculated, then every query passes ad
Got it, thank you for the explanation.


http://gerrit.cloudera.org:8080/#/c/21777/6/tests/query_test/test_sort.py
File tests/query_test/test_sort.py:

http://gerrit.cloudera.org:8080/#/c/21777/6/tests/query_test/test_sort.py@115
PS6, Line 115:     options = [('2g', '100m', '0'), ('400m', '130m', '5')]
I'd suggest to rewrite into this for clarity:

options = [('2g', '100m', '0'), ('400m', '130m', '4')]
if (exec_option['max_sort_run_size'] == 0):
  # max_sort_run_size > 0 will spill more in Case 2.
  options = [('2g', '100m', '0'), ('400m', '130m', '5')]

or

# max_sort_run_size > 0 will spill more in Case 2.
options = [('2g', '100m', '0'),
           ('400m', '130m', ('5' if exec_option['max_sort_run_size'] > 0 else '4'))]


http://gerrit.cloudera.org:8080/#/c/21777/6/tests/query_test/test_sort.py@167
PS6, Line 167: # With 2 pages per run the varlen data is more fragmented and requires a higher limit.
# With max_sort_run_size=2 (2 pages per run) the varlen data is more fragmented and requires a higher limit to maintain "TotalMergesPerformed: 1" assertion.


http://gerrit.cloudera.org:8080/#/c/21777/6/tests/query_test/test_sort.py@243
PS6, Line 243: The larger the run size, the sooner the sorter can give up memory to the next node.
Move this comment just after L245. And maybe reword to this:

"If max_sort_run_size > 0, the larger the run size, the sooner the sorter can give up memory to the next node."


http://gerrit.cloudera.org:8080/#/c/21777/6/tests/query_test/test_sort.py@246
PS6, Line 246: buffer_limit = '27m'
Add comment:

# Increase buffer_limit to maintain such that query never spill.


http://gerrit.cloudera.org:8080/#/c/21777/6/tests/query_test/test_sort.py@250
PS6, Line 250: buffer_limit
"buffer_pool_limit", for consistency.


http://gerrit.cloudera.org:8080/#/c/21777/6/tests/query_test/test_sort.py@250
PS6, Line 250: $LIMIT
"$BUFFER_POOL_LIMIT", for consistency.


http://gerrit.cloudera.org:8080/#/c/21777/6/tests/query_test/test_sort.py@402
PS6, Line 402: # With 2 pages per run the var-len data is more fragmented, therefore it spills more.
# With max_sort_run_size=2 (2 pages per run) the var-len data is more fragmented, therefore it spills more.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I943d8edcc87df168448a174d6c9c6b46fe960eae
Gerrit-Change-Number: 21777
Gerrit-PatchSet: 6
Gerrit-Owner: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 15:10:53 +0000
Gerrit-HasComments: Yes