Matthew Jacobs has posted comments on this change. Change subject: Improve Kudu UPSERT test coverage ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/4953/2/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: PS2, Line 341: : s Right now I think this would need more work to support both a RESULTS section and a DML_RESULTS section, see my comments in test_result_verifier.py. For now, let's assert in here (i.e. RESULTS is set) that DML_RESULTS isn't also set to make sure a test case doesn't try to specify both. PS2, Line 357: target_impalad_client = choice(target_impalad_clients) remove; this is already defined above on l310 PS2, Line 359: self.__verify_results_and_errors(vector, test_section, dml_result, use_db, : 'DML_RESULTS') 1) add a DCHECK that ERRORS isn't specified because we shouldn't be checking them against the DML_RESULTS query. 2) Revert __verify_results_and_errors and instead just call the verify_raw_results method directly: verify_raw_results(test_section, result, vector.get_value('table_format').file_format, pytest.config.option.update_results, replace_filenames_with_placeholder, results_section) http://gerrit.cloudera.org:8080/#/c/4953/2/tests/common/test_result_verifier.py File tests/common/test_result_verifier.py: PS2, Line 288: verify_raw_results Right now this gets confusing when there are DML_RESULTS and other sections, e.g. ERRORS, TYPES, etc., anything checked in this method. This code ends up assuming that any other section applies to 'results_section'. That happens to work right now because none of the test cases have both 'RESULTS' and 'DML_RESULTS'. If there were an ERRORS section that is intended to apply to the actual DML stmt, then checking the table results after in the DML_RESULTS section would break since it wouldn't have those errors. The best thing to do would be to split apart some of this functionality since it's doing a bunch of stuff (we already have verify_results() and verify_errors()), then have a separate method that can handle verifying dml results nicely, e.g. all of the sections including RESULTS _and_ DML_RESULTS, where ERRORS applies to RESULTS; TYPES/LABELS applies to DML_RESULTS. However, in the interest of getting tests in sooner rather than later, we should at least ensure that this isn't misused and it's commented well enough. The comment in the header should: 1) make it clear that this is checking ERRORS, TYPES, LABELS, and result_section against the exec_result. Mention that result_section is a parameter for DML tests so a follow-up SELECT can verify the final state of the table. 2) leave a TODO to split this fn up and have better tailored methods for checking regular select query results vs checking DML results. -- To view, visit http://gerrit.cloudera.org:8080/4953 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib9e7afbef60186edb00a9d11fbe5a8c64931add6 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
