impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
Subject [Impala-ASF-CR] Improve Kudu UPSERT test coverage
Date Tue, 08 Nov 2016 04:15:35 GMT
Matthew Jacobs has posted comments on this change.

Change subject: Improve Kudu UPSERT test coverage

Patch Set 2:


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.
Commit Message:

PS2, Line 9: lease
... 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

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.
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).
File tests/common/

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
To unsubscribe, visit

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

View raw message