You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org> on 2022/08/02 16:06:56 UTC
[Impala-ASF-CR] IMPALA-10918: Allow map type in SELECT list
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 <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Aug 2022 16:06:56 +0000
Gerrit-HasComments: Yes