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 Marshall (Code Review)" <ge...@cloudera.org> on 2019/01/11 21:12:05 UTC

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files 

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


Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................

IMPALA-5847: Fix incorrect use of SET in .test files

The '.test' files are used to run queries for tests. These files are
run with a vector of default query options. They also sometimes
include SET queries that modify query options. If SET is used on a
query option that is included in the vector, the default value from
the vector will override the value from the SET, leading to tests that
don't actually run with the query options they appear to.

This patch asserts that '.test' files don't use SET for values present
in the default vector. It also fixes various tests that already had
this incorrect behavior.

Testing:
- Passed a full exhaustive run.

Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
---
M testdata/workloads/functional-query/queries/QueryTest/codegen-mem-limit.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test
A testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan.test
A testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit.test
A testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive-no-default-buffer-size.test
M testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive.test
A testdata/workloads/functional-query/queries/QueryTest/subquery-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/subquery.test
M testdata/workloads/tpch/queries/sort-reservation-usage.test
M tests/common/impala_test_suite.py
M tests/common/test_dimensions.py
M tests/query_test/test_join_queries.py
M tests/query_test/test_mt_dop.py
M tests/query_test/test_nested_types.py
M tests/query_test/test_queries.py
M tests/query_test/test_scanners.py
M tests/query_test/test_spilling.py
19 files changed, 163 insertions(+), 118 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_join_queries.py
File tests/query_test/test_join_queries.py:

http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_join_queries.py@63
PS1, Line 63:  
flake8: E261 at least two spaces before inline comment


http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_mt_dop.py
File tests/query_test/test_mt_dop.py:

http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_mt_dop.py@45
PS1, Line 45:  
flake8: E261 at least two spaces before inline comment


http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_scanners.py@322
PS1, Line 322:  
flake8: E261 at least two spaces before inline comment



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Jan 2019 21:13:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 5: Code-Review+2

carrying forward


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Jan 2019 21:16:42 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Brown, David Knupp, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................

IMPALA-5847: Fix incorrect use of SET in .test files

The '.test' files are used to run queries for tests. These files are
run with a vector of default query options. They also sometimes
include SET queries that modify query options. If SET is used on a
query option that is included in the vector, the default value from
the vector will override the value from the SET, leading to tests that
don't actually run with the query options they appear to.

This patch asserts that '.test' files don't use SET for values present
in the default vector. It also fixes various tests that already had
this incorrect behavior.

Testing:
- Passed a full exhaustive run.

Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
---
M testdata/workloads/functional-query/queries/QueryTest/codegen-mem-limit.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test
A testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan.test
A testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit.test
A testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive-no-default-buffer-size.test
M testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive.test
A testdata/workloads/functional-query/queries/QueryTest/subquery-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/subquery.test
M testdata/workloads/tpch/queries/sort-reservation-usage.test
M tests/common/impala_test_suite.py
M tests/common/test_dimensions.py
M tests/query_test/test_join_queries.py
M tests/query_test/test_mt_dop.py
M tests/query_test/test_nested_types.py
M tests/query_test/test_queries.py
M tests/query_test/test_scanners.py
M tests/query_test/test_spilling.py
19 files changed, 163 insertions(+), 118 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 2:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Jan 2019 21:54:49 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 4: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Jan 2019 01:49:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 5:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Jan 2019 21:16:32 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Brown, David Knupp, Tim Armstrong, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................

IMPALA-5847: Fix incorrect use of SET in .test files

The '.test' files are used to run queries for tests. These files are
run with a vector of default query options. They also sometimes
include SET queries that modify query options. If SET is used on a
query option that is included in the vector, the default value from
the vector will override the value from the SET, leading to tests that
don't actually run with the query options they appear to.

This patch asserts that '.test' files don't use SET for values present
in the default vector. It also fixes various tests that already had
this incorrect behavior.

Testing:
- Passed a full exhaustive run.

Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
---
M testdata/workloads/functional-query/queries/QueryTest/codegen-mem-limit.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test
A testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan.test
A testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit.test
A testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive-no-default-buffer-size.test
M testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive.test
A testdata/workloads/functional-query/queries/QueryTest/subquery-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/subquery.test
R testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test
M tests/common/impala_test_suite.py
M tests/common/test_dimensions.py
M tests/query_test/test_join_queries.py
M tests/query_test/test_mt_dop.py
M tests/query_test/test_nested_types.py
M tests/query_test/test_queries.py
M tests/query_test/test_scanners.py
M tests/query_test/test_spilling.py
19 files changed, 165 insertions(+), 118 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 4:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Jan 2019 22:00:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 1:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Jan 2019 21:46:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Jan 2019 21:50:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Jan 2019 22:00:03 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 3:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Jan 2019 21:02:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Jan 2019 21:19:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 2:

(6 comments)

No issues with implementation. I looked into whether pytest_generate_tests could be made to ensure copies of vectors, but I think where the vectors are inserted won't permit that. Anyway...

I left some comments that are suggestions about making things a little clearer for someone else developing in these .test files, or if someone encounters a failure when they use a query option in a .test file when they shouldn't.

http://gerrit.cloudera.org:8080/#/c/12220/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12220/2//COMMIT_MSG@20
PS2, Line 20: Testing:
            : - Passed a full exhaustive run.
Nice!


http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan-single-node.test
File testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan-single-node.test:

http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan-single-node.test@8
PS2, Line 8: # code path because a single scan node instance must process all input files.
Does it make sense to say in this test file that num_nodes=1 is expected to be set by the Python test?


http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit-single-node.test
File testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit-single-node.test:

http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit-single-node.test@9
PS2, Line 9: # mem_limit is tuned for a 3-node HDFS minicluster.
Same here: does it make sense to say in this test file that num_nodes=1 is expected to be set by the Python test?


http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/subquery-single-node.test
File testdata/workloads/functional-query/queries/QueryTest/subquery-single-node.test:

http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/subquery-single-node.test@4
PS2, Line 4: # executed on a single node.
Same here: does it make sense to say in this test file that num_nodes=1 is expected to be set by the Python test?


http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/tpch/queries/sort-reservation-usage.test
File testdata/workloads/tpch/queries/sort-reservation-usage.test:

http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/tpch/queries/sort-reservation-usage.test@6
PS2, Line 6: # the scan uses less reservation.
Same here: does it make sense to say in this test file that num_nodes=1 is expected to be set by the Python test?

Also, should this file be renamed to declare that it's "single-node" like you did when you broke out other .test files?


http://gerrit.cloudera.org:8080/#/c/12220/2/tests/common/impala_test_suite.py
File tests/common/impala_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/12220/2/tests/common/impala_test_suite.py@468
PS2, Line 468:             assert set_pattern_match.groups()[0] not in vector.get_value("exec_option"), \
             :                 "You cannot set a value in a '.test' file if it is in the test vector."
Will this print the errant query option when it fails? We want to make sure that happens so that failures can be fixed easily.

You could also extend the message to try to help the user fix the issue. I'm not sure how to word it to help and also be short. Maybe you could point to examples of other tests that do the vector deepcopy and then remove/add to the vector trick? Just throwing something out there.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Jan 2019 23:11:23 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_join_queries.py
File tests/query_test/test_join_queries.py:

http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_join_queries.py@63
PS1, Line 63:  
> flake8: E261 at least two spaces before inline comment
Done


http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_mt_dop.py
File tests/query_test/test_mt_dop.py:

http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_mt_dop.py@45
PS1, Line 45:  
> flake8: E261 at least two spaces before inline comment
Done


http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/12220/1/tests/query_test/test_scanners.py@322
PS1, Line 322:  
> flake8: E261 at least two spaces before inline comment
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Jan 2019 21:19:17 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 5:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Jan 2019 21:48:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Brown, David Knupp, Tim Armstrong, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................

IMPALA-5847: Fix incorrect use of SET in .test files

The '.test' files are used to run queries for tests. These files are
run with a vector of default query options. They also sometimes
include SET queries that modify query options. If SET is used on a
query option that is included in the vector, the default value from
the vector will override the value from the SET, leading to tests that
don't actually run with the query options they appear to.

This patch asserts that '.test' files don't use SET for values present
in the default vector. It also fixes various tests that already had
this incorrect behavior.

Testing:
- Passed a full exhaustive run.

Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
---
M testdata/workloads/functional-query/queries/QueryTest/codegen-mem-limit.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test
A testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan.test
A testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit.test
A testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive-no-default-buffer-size.test
M testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive.test
A testdata/workloads/functional-query/queries/QueryTest/subquery-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/subquery.test
R testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test
M tests/common/impala_test_suite.py
M tests/common/test_dimensions.py
M tests/query_test/test_join_queries.py
M tests/query_test/test_mt_dop.py
M tests/query_test/test_nested_types.py
M tests/query_test/test_queries.py
M tests/query_test/test_scanners.py
M tests/query_test/test_sort.py
M tests/query_test/test_spilling.py
20 files changed, 166 insertions(+), 119 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 5: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Jan 2019 01:20:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................

IMPALA-5847: Fix incorrect use of SET in .test files

The '.test' files are used to run queries for tests. These files are
run with a vector of default query options. They also sometimes
include SET queries that modify query options. If SET is used on a
query option that is included in the vector, the default value from
the vector will override the value from the SET, leading to tests that
don't actually run with the query options they appear to.

This patch asserts that '.test' files don't use SET for values present
in the default vector. It also fixes various tests that already had
this incorrect behavior.

Testing:
- Passed a full exhaustive run.

Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Reviewed-on: http://gerrit.cloudera.org:8080/12220
Reviewed-by: Thomas Marshall <th...@cmu.edu>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M testdata/workloads/functional-query/queries/QueryTest/codegen-mem-limit.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test
A testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan.test
A testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit.test
A testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive-no-default-buffer-size.test
M testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive.test
A testdata/workloads/functional-query/queries/QueryTest/subquery-single-node.test
M testdata/workloads/functional-query/queries/QueryTest/subquery.test
R testdata/workloads/tpch/queries/sort-reservation-usage-single-node.test
M tests/common/impala_test_suite.py
M tests/common/test_dimensions.py
M tests/query_test/test_join_queries.py
M tests/query_test/test_mt_dop.py
M tests/query_test/test_nested_types.py
M tests/query_test/test_queries.py
M tests/query_test/test_scanners.py
M tests/query_test/test_sort.py
M tests/query_test/test_spilling.py
20 files changed, 166 insertions(+), 119 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5847: Fix incorrect use of SET in .test files

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

Change subject: IMPALA-5847: Fix incorrect use of SET in .test files
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan-single-node.test
File testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan-single-node.test:

http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan-single-node.test@8
PS2, Line 8: # code path because a single scan node instance must process all input files.
> Does it make sense to say in this test file that num_nodes=1 is expected to
Done


http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit-single-node.test
File testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit-single-node.test:

http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch-mem-limit-single-node.test@9
PS2, Line 9: # mem_limit is tuned for a 3-node HDFS minicluster.
> Same here: does it make sense to say in this test file that num_nodes=1 is 
Done


http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/subquery-single-node.test
File testdata/workloads/functional-query/queries/QueryTest/subquery-single-node.test:

http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/functional-query/queries/QueryTest/subquery-single-node.test@4
PS2, Line 4: # executed on a single node.
> Same here: does it make sense to say in this test file that num_nodes=1 is 
Done


http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/tpch/queries/sort-reservation-usage.test
File testdata/workloads/tpch/queries/sort-reservation-usage.test:

http://gerrit.cloudera.org:8080/#/c/12220/2/testdata/workloads/tpch/queries/sort-reservation-usage.test@6
PS2, Line 6: # the scan uses less reservation.
> Same here: does it make sense to say in this test file that num_nodes=1 is 
Done


http://gerrit.cloudera.org:8080/#/c/12220/2/tests/common/impala_test_suite.py
File tests/common/impala_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/12220/2/tests/common/impala_test_suite.py@468
PS2, Line 468:             assert set_pattern_match.groups()[0] not in vector.get_value("exec_option"), \
             :                 "You cannot set a value in a '.test' file if it is in the test vector."
> Will this print the errant query option when it fails? We want to make sure
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4c0f31bf4850642b624acdb1f6cb8837957990
Gerrit-Change-Number: 12220
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Jan 2019 20:26:42 +0000
Gerrit-HasComments: Yes