Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18736 )
Change subject: IMPALA-10918: Allow map type in SELECT list ...................................................................... Patch Set 6: (11 comments) http://gerrit.cloudera.org:8080/#/c/18736/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18736/6//COMMIT_MSG@1 PS6, Line 1: (IMPALA-11434: Fix analysis of multiple more than 1d arrays in select list) Can you add similar tests as the ones in https://gerrit.cloudera.org/#/c/18734/ ? Probably a new test table would be also needed that has more then one deeply nested maps, e.g. map<int,map<int,string>> http://gerrit.cloudera.org:8080/#/c/18736/6//COMMIT_MSG@8 PS6, Line 8: Can you add that arrays and maps nested in each other are supported? (so only collections in structs / structs in collections are unsupported) http://gerrit.cloudera.org:8080/#/c/18736/6//COMMIT_MSG@12 PS6, Line 12: Can you list the main limitations similarly to https://gerrit.cloudera.org/#/c/17811/, or point to that commit and state that this has similar limitations? (for example no ORDER BY) http://gerrit.cloudera.org:8080/#/c/18736/6//COMMIT_MSG@16 PS6, Line 16: BE I don't see any BE tests, only EE tests http://gerrit.cloudera.org:8080/#/c/18736/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java: http://gerrit.cloudera.org:8080/#/c/18736/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java@a124 PS6, Line 124: : : : : This still returns some kind of error, right? Can we keep the test and only change the error message? http://gerrit.cloudera.org:8080/#/c/18736/6/testdata/workloads/functional-query/queries/QueryTest/nested-map-in-select-list.test File testdata/workloads/functional-query/queries/QueryTest/nested-map-in-select-list.test: http://gerrit.cloudera.org:8080/#/c/18736/6/testdata/workloads/functional-query/queries/QueryTest/nested-map-in-select-list.test@98 PS6, Line 98: UNION currently crashes Impala ouch, I left this in for arrays :) There is no crash as it is rejected in FE http://gerrit.cloudera.org:8080/#/c/18736/6/testdata/workloads/functional-query/queries/QueryTest/ranger_column_masking_struct_in_select_list.test File testdata/workloads/functional-query/queries/QueryTest/ranger_column_masking_struct_in_select_list.test: http://gerrit.cloudera.org:8080/#/c/18736/6/testdata/workloads/functional-query/queries/QueryTest/ranger_column_masking_struct_in_select_list.test@51 PS6, Line 51: ## TODO: Review question: do we need this for maps too? yes http://gerrit.cloudera.org:8080/#/c/18736/6/testdata/workloads/functional-query/queries/QueryTest/ranger_column_masking_struct_in_select_list.test@66 PS6, Line 66: ## TODO: Review question: do we need this for maps too? yes http://gerrit.cloudera.org:8080/#/c/18736/6/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/18736/6/tests/authorization/test_ranger.py@1790 PS6, Line 1790: # TODO: Review question: do we need this for maps too? : TestRanger._add_column_masking_policy( Yes, I think that it is better to have some kind of coverage for this. http://gerrit.cloudera.org:8080/#/c/18736/6/tests/authorization/test_ranger.py@1792 PS6, Line 1792: unique_name + str(policy_cnt), user, "functional_orc_def", : "complextypestbl", "int_array_map", "MASK_NULL") nit: +2 indentation http://gerrit.cloudera.org:8080/#/c/18736/6/tests/query_test/test_nested_types.py File tests/query_test/test_nested_types.py: http://gerrit.cloudera.org:8080/#/c/18736/6/tests/query_test/test_nested_types.py@205 PS6, Line 205: ImpalaTestDimension('mt_dop', 0, 2)) : cls.ImpalaTestMatrix.add_dimension( : create_exec_option_dimension_from_dict({ : 'disable_codegen': ['False', 'True']})) nit: more indentation -- To view, visit http://gerrit.cloudera.org:8080/18736 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I921c647f1779add36e7f5df4ce6ca237dcfaf001 Gerrit-Change-Number: 18736 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Tue, 02 Aug 2022 16:06:56 +0000 Gerrit-HasComments: Yes
