impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] Improve Kudu UPSERT test coverage
Date Tue, 08 Nov 2016 19:24:35 GMT
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 <tmarshall@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message