Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17638 )
Change subject: WiP: IMPALA-9495: Support struct in select list for ORC tables ...................................................................... Patch Set 4: (15 comments) Looks good! Will find time to review tests. http://gerrit.cloudera.org:8080/#/c/17638/4/be/src/exec/orc-column-readers.h File be/src/exec/orc-column-readers.h: http://gerrit.cloudera.org:8080/#/c/17638/4/be/src/exec/orc-column-readers.h@626 PS4, Line 626: was given in the select list of a query nit. May change to "is to be materialized". http://gerrit.cloudera.org:8080/#/c/17638/4/be/src/exec/orc-column-readers.h@627 PS4, Line 627: is_given_in_select_list_ nit. Maybe renamed as is_to_be_materialized. http://gerrit.cloudera.org:8080/#/c/17638/4/be/src/exec/orc-column-readers.cc File be/src/exec/orc-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/17638/4/be/src/exec/orc-column-readers.cc@493 PS4, Line 493: return nit. I am a little confusing by the return here. Does it mean that we only read the scalar components in this tuple, when is_given_in_select_list_ is true? Can you please explain the semantics of is_given_in_select_list_? http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1019 PS4, Line 1019: lower case nit. in lower case. http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/SlotRef.java File fe/src/main/java/org/apache/impala/analysis/SlotRef.java: http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@164 PS4, Line 164: expect nit. expects http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@167 PS4, Line 167: handleStructSlotRef nit. Maybe renamed to expandSlotRefForStruct(). http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@180 PS4, Line 180: checkForUnsupportedFields nit. May renamed to checkForUnsupportedFieldsForStruct(). http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java File fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java: http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@217 PS4, Line 217: setParentSlot nit. Maybe renamed to setParentSlotDesc(). http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@223 PS4, Line 223: getParentSlot nit. Maybe renamed to getParentSlotDesc(). http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@239 PS4, Line 239: if (parentStructSlot_ != null) { : toStrHelper.add("parentSlot", parentStructSlot_.getId()); : } Is it possible to get more info about children? http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@304 PS4, Line 304: Pair<Integer, Integer> nit. May explain the semantics of the pair in comment. http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@341 PS4, Line 341: the null indicator will be on the top level tuple Will this be a problem when some field in a struct is null? Looks like NULLs are allowed within a struct in ORC. See https://orc.apache.org/docs/types.html and https://www.vertica.com/docs/10.1.x/HTML/Content/Authoring/ExternalTables/StructsORC.htm http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@428 PS4, Line 428: sets nit. adds http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@430 PS4, Line 430: setAvgSerializedSize nit. May be renamed as addToAvgSerializedSize(). http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@448 PS4, Line 448: slotDesc.getIsNullable() My impression is that if struct is not nullable, some of its children can still be nullable. -- To view, visit http://gerrit.cloudera.org:8080/17638 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0fbe56bdcd372b72e99c0195d87a818e7fa4bc3a Gerrit-Change-Number: 17638 Gerrit-PatchSet: 4 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Comment-Date: Thu, 29 Jul 2021 19:39:13 +0000 Gerrit-HasComments: Yes
