Alex Behm has posted comments on this change. Change subject: IMPALA-4828: Alter Kudu schema outside Impala may crash on read ......................................................................
Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/5840/2/be/src/exec/kudu-scanner.cc File be/src/exec/kudu-scanner.cc: Line 149: > Good point, though I'll just leave this as a TODO for now because we don't I think Thomas is right and the query may crash if there is a difference in column nullability. We do rely on the nullability. The SlotDescriptors are set to nullable or non-nullable which affects the memory layout of the tuple. If the tuples returned by Kudu are not 100% identical to what Impala expects, you may get non-deterministic wrong results (and maybe a crash). See the class comment in TupleDescriptor.java regarding the Kudu layout. In response to one of your comments: "After discussing with Dan on the Kudu team, we can make this simpler by checking at plan time because the kudu scan token encodes the col metadata and deserializing it will fail if the projection schema is no longer valid. The issue for us was that we did not check the Kudu col type matched our col type at plan time." Will the Kudu-side validation check that the projection is 100% identical to what the scan token expects? Nullability etc. included? http://gerrit.cloudera.org:8080/#/c/5840/6/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java: Line 161: if (!colType.matchesType(kuduColType)) { Shouldn't this use equals() instead of matchesType()? -- 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: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
