Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5799: Kudu DML can crash if schema has changed ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/7688/1//COMMIT_MSG Commit Message: PS1, Line 14: its > Done my bad, I meant for this comment to be on l10 which you changed http://gerrit.cloudera.org:8080/#/c/7688/2//COMMIT_MSG Commit Message: PS2, Line 11: ' this one should be removed http://gerrit.cloudera.org:8080/#/c/7688/2/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: PS2, Line 146: udu_table_sink_.referenced_columns.back() wouldn't this not work when the insert specifies all columns, i.e. referenced_cols is empty() ? PS2, Line 146: if (table_->schema().num_columns() <= kudu_table_sink_.referenced_columns.back()) { : return Status(strings::Substitute( : "Table $0 has fewer columns than expected.", table_desc_->name())); : } this makes me a bit nervous because it seems like expecting the list is in ascending order of col position an implementation detail that could change/break, and I don't think we really rely on it elsewhere or test it explicitly. I think you can just skip this check here, and instead just check right before l153 that table_->schema().num_columns() > col. We don't have to worry about doing a bit more work since this only happens on Open() PS2, Line 151: col nit: col_idx maybe http://gerrit.cloudera.org:8080/#/c/7688/2/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: Line 412: time.sleep(random.random()) comment that this sleeps for up to a second PS2, Line 414: client = self.create_impala_client() nit: move this outside of the loop -- To view, visit http://gerrit.cloudera.org:8080/7688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9fd6bf164310df0041144f75f5ee722665e9f587 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
