Gabor Kaszab 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 10: (7 comments) Thanks for the quick turnaround! I hope I addressed all the latest comments. http://gerrit.cloudera.org:8080/#/c/17638/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17638/4//COMMIT_MSG@58 PS4, Line 58: ingVal. > Okay. I think it is okay to restrict the description to whatever is availab Sure, done http://gerrit.cloudera.org:8080/#/c/17638/4//COMMIT_MSG@64 PS4, Line 64: for each of the struct's children. The struct SlotDescriptor points to > Maybe a summary of the new tests added is sufficient. Done 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 > Okay. Good explanation. Note my other comment in the header file to drop 'is_given_in_select_list_' and use the existing 'materialize_tuple_' for the same purpose. http://gerrit.cloudera.org:8080/#/c/17638/4/be/src/runtime/raw-value.cc File be/src/runtime/raw-value.cc: http://gerrit.cloudera.org:8080/#/c/17638/4/be/src/runtime/raw-value.cc@223 PS4, Line 223: DCHECK(value != nullptr && tuple != nullptr && slot_desc != nullptr && > Or at least we could add a DCHECK. At the callsite (tuple.cc:235) it is needed to do the check so that we can set the primitive slots (or the outmost struct slot) to null if 'value' is null. I've already added a DCHECK here to verify 'value' is not null. http://gerrit.cloudera.org:8080/#/c/17638/7/be/src/udf/udf.h File be/src/udf/udf.h: http://gerrit.cloudera.org:8080/#/c/17638/7/be/src/udf/udf.h@740 PS7, Line 740: > I wonder if we could potentially follow the same model for other data types I have no strong opinions on either way. Currently, I don;t see what an additional layer could add, and still wouldn't make sense to expose StructVal in udf.h as we should wait for a few releases so that this can mature a little bit. 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@304 PS4, Line 304: Pair<Integer, Integer> > Okay. Thanks for pointing it out. Maybe make that sentence more clear: "Ret Sure, Done http://gerrit.cloudera.org:8080/#/c/17638/4/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@341 PS4, Line 341: truct at the top level (i.e., parentStructSlot_ = > Thanks for the explanation. I think we can say something like the following Thanks for the suggestion! Done -- 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: 10 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Mon, 23 Aug 2021 12:44:37 +0000 Gerrit-HasComments: Yes
