Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/18863 )
Change subject: IMPALA-9499: Display support for all complex types in a SELECT * query ...................................................................... Patch Set 5: (15 comments) Thanks! http://gerrit.cloudera.org:8080/#/c/18863/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18863/5//COMMIT_MSG@9 PS5, Line 9: This change adds EXPAND_COMPLEX_TYPES query option to support the We should also mention somewhere in the commit message that the by default the new query option is turned off because it would be a breaking change. http://gerrit.cloudera.org:8080/#/c/18863/5//COMMIT_MSG@15 PS5, Line 15: collection Collection usually refers to array and map types, excluding structs, so we should omit this word here. http://gerrit.cloudera.org:8080/#/c/18863/5//COMMIT_MSG@18 PS5, Line 18: - Analyzer tests check the proper expansion of complex types when the This can be misunderstood; Analyzer tests only check whether the query returns an error or not, they cannot check whether the expansion is correct. Could you make it clearer? http://gerrit.cloudera.org:8080/#/c/18863/5/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/18863/5/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@a809 PS5, Line 809: : : : : Are you sure we should delete this? I don't think it's about expanding complex types but what the 'root' of the start expression is, i.e. select * from tbl OR select struct.* from tbl. http://gerrit.cloudera.org:8080/#/c/18863/5/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/18863/5/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@a79 PS5, Line 79: Is this change needed because we want to add a relative path in SelectStmt.addStarResultExpr()? http://gerrit.cloudera.org:8080/#/c/18863/5/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/18863/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1077 PS5, Line 1077: ctx.getQueryOptions().setDisable_codegen(true); Could leave a TODO that this setting will no longer be necessaty once IMPALA-10851 is resolved. http://gerrit.cloudera.org:8080/#/c/18863/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1079 PS5, Line 1079: AnalyzesOk("select * from functional_parquet.complextypes_structs",ctx); All AnalyzeOk tests pass also before this commit, the queries simply omit the complex types. I don't think these add value. http://gerrit.cloudera.org:8080/#/c/18863/5/testdata/workloads/functional-query/queries/QueryTest/nested-types-star-expansion-disabled.test File testdata/workloads/functional-query/queries/QueryTest/nested-types-star-expansion-disabled.test: http://gerrit.cloudera.org:8080/#/c/18863/5/testdata/workloads/functional-query/queries/QueryTest/nested-types-star-expansion-disabled.test@1 PS5, Line 1: ==== There should be a comment noting that this file should be kept in sync with testdata/workloads/functional-query/queries/QueryTest/nested-types-star-expansion-array.test in case one of the files is modified later. http://gerrit.cloudera.org:8080/#/c/18863/5/testdata/workloads/functional-query/queries/QueryTest/nested-types-star-expansion-disabled.test@3 PS5, Line 3: with disabled query option "with EXPAND_COMPLEX_TYPES disabled" would be more informative. Same on the other queries. http://gerrit.cloudera.org:8080/#/c/18863/5/testdata/workloads/functional-query/queries/QueryTest/nested-types-star-expansion-struct.test File testdata/workloads/functional-query/queries/QueryTest/nested-types-star-expansion-struct.test: http://gerrit.cloudera.org:8080/#/c/18863/5/testdata/workloads/functional-query/queries/QueryTest/nested-types-star-expansion-struct.test@45 PS5, Line 45: select alltypes.* from complextypes_structs Can we have a query where both a struct and an array is used as the root of a star? E.g. select struct.*, array.* from table; I think there are tables where both structs and arrays are present - if not, we could join them. http://gerrit.cloudera.org:8080/#/c/18863/5/tests/query_test/test_nested_types.py File tests/query_test/test_nested_types.py: http://gerrit.cloudera.org:8080/#/c/18863/5/tests/query_test/test_nested_types.py@155 PS5, Line 155: T I think this T was added by mistake, we could remove it now that we're touching this file. http://gerrit.cloudera.org:8080/#/c/18863/5/tests/query_test/test_nested_types.py@878 PS5, Line 878: Type Types (plural) would be better, we use that in other test classes in this file. http://gerrit.cloudera.org:8080/#/c/18863/5/tests/query_test/test_nested_types.py@902 PS5, Line 902: vector.get_value('exec_option')['disable_codegen'] = 'true' We could leave a TODO that this is not needed once IMPALA-10851 is resolved. http://gerrit.cloudera.org:8080/#/c/18863/5/tests/query_test/test_nested_types.py@905 PS5, Line 905: " This should be a regular comment, e.g. a line starting with #. AFAIK triple-quote strings are only used in doc comments at the top of the function. http://gerrit.cloudera.org:8080/#/c/18863/5/tests/query_test/test_nested_types.py@911 PS5, Line 911: """ See L905. -- To view, visit http://gerrit.cloudera.org:8080/18863 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84b5e5703f9e0ce0f4f8bff83941677dd7489974 Gerrit-Change-Number: 18863 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Fri, 26 Aug 2022 13:35:01 +0000 Gerrit-HasComments: Yes
