Alex Behm has posted comments on this change. Change subject: IMPALA-3938: Prevent illegal implicit collection references being used. ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/4079/2/fe/src/main/java/com/cloudera/impala/analysis/Path.java File fe/src/main/java/com/cloudera/impala/analysis/Path.java: Line 227: && isOptionalFieldCollectionType((CollectionStructType)structType); Based on this latest version I think the code can be simplified to not even use expectExplicitMatch at all, as follows: // Map all remaining raw-path elements to field types and positions. while (rawPathIdx < rawPath_.size()) { if (!currentType.isComplexType()) return false; StructType structType = getTypeAsStruct(currentType); // Resolve explicit path. StructField field = structType.getField(rawPath_.get(rawPathIdx)); if (field == null) { // Resolve implicit path. if (structType instanceof CollectionStructType) { field = ((CollectionStructType) structType).getOptionalField(); // Collections must be matched explicitly. if (field.getType().isCollectionType()) return false; } else { // Failed to resolve implicit or explicit path. return false; } // Update the physical types/positions. matchedTypes_.add(field.getType()); matchedPositions_.add(field.getPosition()); currentType = field.getType(); // Do not consume a raw-path element. continue; } matchedTypes_.add(field.getType()); matchedPositions_.add(field.getPosition()); if (field.getType().isCollectionType() && firstCollectionPathIdx_ == -1) { Preconditions.checkState(firstCollectionTypeIdx_ == -1); firstCollectionPathIdx_ = rawPathIdx; firstCollectionTypeIdx_ = matchedTypes_.size() - 1; } currentType = field.getType(); ++rawPathIdx; } http://gerrit.cloudera.org:8080/#/c/4079/2/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java: Line 597: // Test implicit key/value reference is not abused when using a map within an array. can you explain which resolution is not allowed? "is not abused" is not very clear In fact, could you rephrase the comment in L578 and followin as well? The underlying issue seems to be that collections must always be matched explicitly. Do you agree? Line 598: addTestTable("create table d.t9 (c1 array<map<string, string>>)"); nit: use 'c' instead of 'c1' for consistency with the other tests Line 599: AnalysisError("select key from d.t9.c1", add similar tests for a map nested inside a map (we can run into the same issue) -- To view, visit http://gerrit.cloudera.org:8080/4079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I703c7e9c8c9c95c9fee714767367373eec10cd0c Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Christopher Channing <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Christopher Channing <[email protected]> Gerrit-HasComments: Yes
