impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5787: Dropped status in KuduTableSink::Send()
Date Tue, 15 Aug 2017 18:03:17 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5787: Dropped status in KuduTableSink::Send()
......................................................................


Patch Set 1: Code-Review+2

(1 comment)

I'm OK with making this change now, but there are a bunch of open questions we should still
address for 2.10. Fine to do that in IMPALA-5799.

http://gerrit.cloudera.org:8080/#/c/7667/1/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

PS1, Line 241:       // This can only fail if we set a col to an incorrect type, which would
be a bug in
             :       // planning, so we can DCHECK.
> I checked with the Kudu team, and as long as the schema is what we want whe
Hm I don't see how the writes fail at Apply() given the writes happen asynchronously. I'd
think that we would see errors in CheckForErrors().

While Kudu doesn't allow col type changes, isn't that accomplished by: a col drop + col add
w/ the same name but a different type?
wrt the comment about adding cols: even w/o non-nullable columns, it seems unsafe to drop
a string col and add another col with the same name but a different type. Seems like there's
a potential for some code to dereference an invalid pointer.

I'm OK with continuing this line of thinking in IMPALA-5799, but we should make sure to address
it for 2.10.


-- 
To view, visit http://gerrit.cloudera.org:8080/7667
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia228d98ca90de8638b0afcc242c788e225fb450f
Gerrit-PatchSet: 1
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-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message