[Impala-ASF-CR] Improve Kudu UPSERT test coverage
Hello Matthew Jacobs, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4953 to look at the new patch set (#5). Change subject: Improve Kudu UPSERT test coverage .. Improve Kudu UPSERT test coverage This patch also introduces a new test section 'DML_RESULTS', which takes the name of a table as a comment and the contents of the table as its body and then verifies that the body matches the actual contents of the table. This makes it easy to check that a DML operation has the desired effect on the contents of a table, rather than always having to add another test case that runs a select on the table. For now, this section cannot be used in a test along with the RESULTS or ERRORS sections. Change-Id: Ib9e7afbef60186edb00a9d11fbe5a8c64931add6 --- M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test A testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/query_test/test_kudu.py M tests/util/test_file_parser.py 6 files changed, 526 insertions(+), 67 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/4953/5 -- To view, visit http://gerrit.cloudera.org:8080/4953 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib9e7afbef60186edb00a9d11fbe5a8c64931add6 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] Improve Kudu UPSERT test coverage
Thomas Tauber-Marshall has posted comments on this change. Change subject: Improve Kudu UPSERT test coverage .. Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/4953/4//COMMIT_MSG Commit Message: PS4, Line 9: This patch also introduces a new test section 'DML_RESULTS', which : takes the name of a table as a comment and the contents of the : table as its body and then verifies that the body > let's remove this; sorry if my previous comments were confusing Done http://gerrit.cloudera.org:8080/#/c/4953/4/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test File testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test: PS4, Line 23: NumModifiedRows: 1 > I realized in my own change that all of these regex checks can be simplifie Done http://gerrit.cloudera.org:8080/#/c/4953/4/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: PS4, Line 341: Combining 'RESULTS' with 'DML_RESULTS" > add a comment that this isn't supported for now because __verify_results_an Done PS4, Line 357: UNTIME_PRO > Please add a comment about the limit, e.g .to make sure the queries aren't Done -- 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: Yes
[Impala-ASF-CR] Improve Kudu UPSERT test coverage
Matthew Jacobs has posted comments on this change. Change subject: Improve Kudu UPSERT test coverage .. Patch Set 4: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/4953/4//COMMIT_MSG Commit Message: PS4, Line 9: In preparation for the public release of Kudu integration in : 2.8, we need to make sure that we've covered as much of the : Kudu related functionality with tests as possible. let's remove this; sorry if my previous comments were confusing I think the line above covers this. http://gerrit.cloudera.org:8080/#/c/4953/4/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test File testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test: PS4, Line 23: row_regex: .*NumModifiedRows: 1.* I realized in my own change that all of these regex checks can be simplified: we don't need the regex we can just do NumModifiedRows: 1 Can you clean these up here and below? http://gerrit.cloudera.org:8080/#/c/4953/4/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: PS4, Line 341: ssert 'DML_RESULTS' not in test_section add a comment that this isn't supported for now because __verify_results_and_errors calls verify_raw_results which always checks ERRORS, TYPES, LABELS, ... and it doesn't make sense to do so for this as well as for DML_RESULTS below. PS4, Line 357: limit 1000 Please add a comment about the limit, e.g .to make sure the queries aren't unbounded; the tests shouldn't be checking tables this big anyway -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: Yes
[Impala-ASF-CR] Improve Kudu UPSERT test coverage
Thomas Tauber-Marshall has uploaded a new patch set (#4). Change subject: Improve Kudu UPSERT test coverage .. Improve Kudu UPSERT test coverage In preparation for the public release of Kudu integration in 2.8, we need to make sure that we've covered as much of the Kudu related functionality with tests as possible. This patch also introduces a new test section 'DML_RESULTS', which takes the name of a table as a comment and the contents of the table as its body and then verifies that the body matches the actual contents of the table. This makes it easy to check that a DML operation has the desired effect on the contents of a table, rather than always having to add another test case that runs a select on the table. For now, this section cannot be used in a test along with the RESULTS or ERRORS sections. Change-Id: Ib9e7afbef60186edb00a9d11fbe5a8c64931add6 --- M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test A testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/query_test/test_kudu.py M tests/util/test_file_parser.py 6 files changed, 502 insertions(+), 63 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/4953/4 -- To view, visit http://gerrit.cloudera.org:8080/4953 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib9e7afbef60186edb00a9d11fbe5a8c64931add6 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] Improve Kudu UPSERT test coverage
Thomas Tauber-Marshall has posted comments on this change. Change subject: Improve Kudu UPSERT test coverage .. Patch Set 4: (3 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: test_section : s > Right now I think this would need more work to support both a RESULTS secti Done PS2, Line 357: dml_results_query = "select * from %s limit 1000" % \ > remove; this is already defined above on l310 Done PS2, Line 359: dml_result = self.__execute_query(target_impalad_client, dml_results_query) : verify_raw_results > 1) add a DCHECK that ERRORS isn't specified because we shouldn't be checkin Done -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: Yes
[Impala-ASF-CR] Improve Kudu UPSERT test coverage
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
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-HasComments: Yes
[Impala-ASF-CR] Improve Kudu UPSERT test coverage
Thomas Tauber-Marshall has uploaded a new patch set (#3). Change subject: Improve Kudu UPSERT test coverage .. Improve Kudu UPSERT test coverage In preparation for the public release of Kudu integration in 2.8, we need to make sure that we've covered as much of the Kudu related functionality with tests as possible. This patch also introduces a new test section 'DML_RESULTS', which takes the name of a table as a comment and the contents of the table as its body and then verifies that the body matches the actual contents of the table. This makes it easy to check that a DML operation has the desired effect on the contents of a table, rather than always having to add another test case that runs a select on the table. Change-Id: Ib9e7afbef60186edb00a9d11fbe5a8c64931add6 --- M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test A testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/query_test/test_kudu.py M tests/util/test_file_parser.py 6 files changed, 498 insertions(+), 66 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/4953/3 -- To view, visit http://gerrit.cloudera.org:8080/4953 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib9e7afbef60186edb00a9d11fbe5a8c64931add6 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] Improve Kudu UPSERT test coverage
Thomas Tauber-Marshall has posted comments on this change. Change subject: Improve Kudu UPSERT test coverage .. Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/4953/2//COMMIT_MSG Commit Message: PS2, Line 9: relea > release Done PS2, Line 10: 2.8, > 2.8 Done PS2, Line 9: In preparation for the public release of Kudu integration in : 2.8, we need to make sure that we've covered as much of the : Kudu related functionality with tests as possible. : > Not really needed in the commit. Done http://gerrit.cloudera.org:8080/#/c/4953/1/testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test: PS1, Line 680: : : : > Ah, makes sense. Thanks for looking into that, I think it's fine. We can th IMPALA-4445 http://gerrit.cloudera.org:8080/#/c/4953/2/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: PS2, Line 356: select * from %s > might be good to put a limit on here for safety, e.g. the number of rows sp Done -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: Yes
[Impala-ASF-CR] Improve Kudu UPSERT test coverage
Matthew Jacobs has posted comments on this change. Change subject: Improve Kudu UPSERT test coverage .. Patch Set 2: (7 comments) Thanks! This is great. I didn't look as closely as I did in revision 1, but I'll check again tomorrow and but can probably give a +1. I'll see if anyone wants to take a look at the python code. Otherwise I'll give it a +2 after I look again. http://gerrit.cloudera.org:8080/#/c/4953/2//COMMIT_MSG Commit Message: PS2, Line 9: lease release ... though I guess it seems odd to _re_ lease the Impala Kudu integration which has never been leased before (at least not GA). PS2, Line 10: 5.10 2.8 PS2, Line 9: In preparation for the public lease of Kudu integration in : 5.10, we need to make sure that we've covered as much of the : Kudu related functionality with tests as possible. This patch : covers UPSERT. Not really needed in the commit. PS2, Line 14: It also introduces a new test section 'DML_RESULTS', which : takes the name of a table as a comment and the contents of the : table as its body and then verifies that the body matches the : actual contents of the table. This makes it easy to check that a : DML operation has the desired effect on the contents of a table, : rather than always having to add another test case that runs a : select on the table. nice http://gerrit.cloudera.org:8080/#/c/4953/1/testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test: Line 433 > How about adding the 'LABELS' section? That's great, I didn't know it existed. PS1, Line 680: : : : > Good point - there appears to only actually be 10 distinct values of int_co Ah, makes sense. Thanks for looking into that, I think it's fine. We can think about changing the wording in the future (not something to pile on now). http://gerrit.cloudera.org:8080/#/c/4953/2/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: PS2, Line 356: select * from %s might be good to put a limit on here for safety, e.g. the number of rows specified in the "expected rows" section, or even just some big constant like 1000. Even if it's kinda hacky/random, it'd be crazy to have a test case where the DML_RESULTS section even got close but would prevent the test from trying to do something dumb. -- 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 Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: Yes
[Impala-ASF-CR] Improve Kudu UPSERT test coverage
Thomas Tauber-Marshall has uploaded a new change for review. http://gerrit.cloudera.org:8080/4953 Change subject: Improve Kudu UPSERT test coverage .. Improve Kudu UPSERT test coverage In preparation for the public lease of Kudu integration in 5.10, we need to make sure that we've covered as much of the Kudu related functionality with tests as possible. This patch covers UPSERT. It also introduces a new test section 'DML_RESULTS', which takes the name of a table as a comment and the contents of the table as its body and then verifies that the body matches the actual contents of the table. This makes it easy to check that a DML operation has the desired effect on the contents of a table, rather than always having to add another test case that runs a select on the table. Change-Id: Ib9e7afbef60186edb00a9d11fbe5a8c64931add6 --- M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test A testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/query_test/test_kudu.py M tests/util/test_file_parser.py 6 files changed, 496 insertions(+), 66 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/4953/2 -- To view, visit http://gerrit.cloudera.org:8080/4953 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib9e7afbef60186edb00a9d11fbe5a8c64931add6 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall
