Jean-Daniel Cryans has posted comments on this change. Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first ......................................................................
Patch Set 1: > It seems like in the context of a projection, we don't want to > label any columns as keys, and thus this probably passes because we > don't propagate the "key" status from the table schema into the > projection schema. If this were causing a problem, wouldn't the > test added in the above-linked commit be failing now? If you think > the coverage is lacking, can you propose a new unit test which > fails with Jun's patch? I just found that we're removing all the key information and whatnot when creating a scanner now, missed that change. So that's why the test I added a while back passes now. > > Really the 'key(true)' vs 'key(false)' API is a bad one that we > should try to deprecate over time in favor of a 'setPrimaryKey(List<String>)' > type API (we've moved towards this in the C++ client). This paves > the way for actually allowing key columns not listed first in the > Schema object. This seems like a better change overall instead of cementing the current situation with more code. -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He <junhe...@gmail.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: No
