Csaba Ringhofer 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 7: (6 comments) Some comments for the FE part. I still don't have a 100% understanding of what happens and why, but I am getting closer :) http://gerrit.cloudera.org:8080/#/c/17638/7/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/7/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@182 PS7, Line 182: StrustType typo http://gerrit.cloudera.org:8080/#/c/17638/7/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/7/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@430 PS7, Line 430: setAvgSerializedSize Can you add some comment about structs? My guess is COMPUTE STATS ignores them, so their STRING members won't have AVG size. http://gerrit.cloudera.org:8080/#/c/17638/7/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@448 PS7, Line 448: getIsNullable If a struct could be non-nullable we would still need to recursively go through it's children. Can you handle this case or add a precondition that all structs are nullable? http://gerrit.cloudera.org:8080/#/c/17638/7/fe/src/main/java/org/apache/impala/catalog/StructType.java File fe/src/main/java/org/apache/impala/catalog/StructType.java: http://gerrit.cloudera.org:8080/#/c/17638/7/fe/src/main/java/org/apache/impala/catalog/StructType.java@99 PS7, Line 99: getSlotSize Can you add a comment about the layout, e.g. "structs are flattened by the backend while the null indicators are handled in the parent tuple, so the struct slot's size will be the some of all member's size. http://gerrit.cloudera.org:8080/#/c/17638/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/17638/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@a2293 PS7, Line 2293: : : : Is this actually supported? My understanding is that we support returning structs from ORC but not from Kudu http://gerrit.cloudera.org:8080/#/c/17638/7/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/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@573 PS7, Line 573: addTestTable("create table d.t6 (c map<int,struct<f:int,key:int,value:int>>) " + : "stored as orc"); Can you add a similar test table with nested structs and check a few SlotRefPaths for it? -- 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: 7 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: Tue, 17 Aug 2021 09:01:00 +0000 Gerrit-HasComments: Yes
