Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201611081924.uA8JOZZ1022053@ip-10-146-233-104.ec2.internal>
Date: Tue, 8 Nov 2016 19:24:35 +0000
From: "Matthew Jacobs (Code Review)" <gerrit@cloudera.org>
To: Thomas Tauber-Marshall <tmarshall@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Reply-To: mj@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_Improve_Kudu_UPSERT_test_coverage=0A?=
In-Reply-To: <gerrit.1478289154000.Ib9e7afbef60186edb00a9d11fbe5a8c64931add6@gerrit.cloudera.org>
References: <gerrit.1478289154000.Ib9e7afbef60186edb00a9d11fbe5a8c64931add6@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Tue, 08 Nov 2016 19:26:49 -0000

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