Daniel Becker 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 3: (8 comments) I can't say I completely understand everything, I've only found some nit. http://gerrit.cloudera.org:8080/#/c/17638/3/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: http://gerrit.cloudera.org:8080/#/c/17638/3/be/src/runtime/buffered-tuple-stream.cc@85 PS3, Line 85: continue Nit: Maybe it would be nicer to use ```if (!condition) {then}``` instead of ```if (condition) continue; then```. Also applies on L89. http://gerrit.cloudera.org:8080/#/c/17638/3/be/src/runtime/types.h File be/src/runtime/types.h: http://gerrit.cloudera.org:8080/#/c/17638/3/be/src/runtime/types.h@264 PS3, Line 264: inline int GetSlotSize(const ColumnType& col_type) const { Nit: this could be static. http://gerrit.cloudera.org:8080/#/c/17638/3/be/src/runtime/types.h@288 PS3, Line 288: inline int GetByteSize(const ColumnType& col_type) const { Nit: this could be static. http://gerrit.cloudera.org:8080/#/c/17638/3/be/src/udf/udf.h File be/src/udf/udf.h: http://gerrit.cloudera.org:8080/#/c/17638/3/be/src/udf/udf.h@747 PS3, Line 747: uint8_t** ptr; Would using std::vector not be compatible? Or too much overhead? http://gerrit.cloudera.org:8080/#/c/17638/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java File fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java: http://gerrit.cloudera.org:8080/#/c/17638/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@248 PS3, Line 248: // For scalar types, the materialized path is the same as path_ Shouldn't we include struct types in the comment? http://gerrit.cloudera.org:8080/#/c/17638/3/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/3/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@164 PS3, Line 164: expect Typo: expects http://gerrit.cloudera.org:8080/#/c/17638/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/17638/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@418 PS3, Line 418: scalar Is it still true or is it a scalar OR a struct? http://gerrit.cloudera.org:8080/#/c/17638/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@978 PS3, Line 978: support Nit: are supported -- 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: 3 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: Wed, 28 Jul 2021 15:28:16 +0000 Gerrit-HasComments: Yes
