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-4828: Alter Kudu schema outside Impala may crash on read
Date Tue, 07 Feb 2017 22:33:48 GMT
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 <mj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message