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 <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 14 Jan 2019 23:11:23 +0000 Gerrit-HasComments: Yes
