Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4828: Alter Kudu schema outside Impala may crash on read ......................................................................
Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/5840/2/be/src/exec/kudu-scanner.cc File be/src/exec/kudu-scanner.cc: Line 149: if (slot->type().type != KuduColTypeToPrimitiveType(col.type())) { > This doesn't address the case where you removed a column and then added a n Good point, though I'll just leave this as a TODO for now because we don't rely on either of these (in fact I think we won't even know about nullability in our tuple descriptor because I don't believe it gets returned by the catalog). http://gerrit.cloudera.org:8080/#/c/5840/2/common/thrift/generate_error_codes.py File common/thrift/generate_error_codes.py: PS2, Line 317: KUDU_COL_MISMATCH > Maybe also explicitly mention refreshing here, as above. Done PS2, Line 317: KUDU_COL_MISMATCH > nit: maybe call this 'KUDU_NUM_COLS_MISMATCH', else it sounds like it could Done http://gerrit.cloudera.org:8080/#/c/5840/2/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: Line 148: > nit: extra line Done PS2, Line 162: cursor.execute("SELECT * FROM %s.foo" % (unique_database)) : assert cursor.fetchall() == [(0, )] > Should we assert that the warning is printed here too? It's a good idea but I don't think we can with the cursor mechanism. It'd be a good thing to add to the cursor, so I'll file a JIRA and leave a TODO here. PS2, Line 191: Column 's' not found in kudu table impala::test_kudu_col_removed > This is not the same as the 'KUDU_COL_MISMATCH' error. Does that mean this Yes you're right. This gets caught during planning. I still added the validation in the scanner to be safe though because it's possible for the schema to change after planning but before the scanner is opened. If that happens, the validation after the scanner opening will catch that. I think it's too hard to catch that case in a test though. Ideally we'd have some test that repeatedly makes DDL changes and issues queries, some sort of stress, but I think that's something we should consider for the future. -- To view, visit http://gerrit.cloudera.org:8080/5840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6d43f5bb9811e728ad592933066d006c8fb4553a Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
