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 <tmarsh...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
