Arnab Karmakar has posted comments on this change. ( http://gerrit.cloudera.org:8080/23709 )
Change subject: IMPALA-13552: Add support for size()/length() for array/map types ...................................................................... Patch Set 4: (14 comments) Sorry to take so long. Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/23709/2/be/src/exprs/collection-functions-test.cc File be/src/exprs/collection-functions-test.cc: http://gerrit.cloudera.org:8080/#/c/23709/2/be/src/exprs/collection-functions-test.cc@34 PS2, Line 34: FunctionContext::TypeDesc param_desc; > Do these tests also need to run against TYPE_MAP? Yes right the tests should run against both FunctionContext types. The test fixture is now parameterized. http://gerrit.cloudera.org:8080/#/c/23709/2/be/src/exprs/collection-functions-test.cc@126 PS2, Line 126: CollectionVal map_collection(map_data, kCollectionSize); > Define a constant instead of using the magic number 5. Replaced with kCollectionSize = 5. http://gerrit.cloudera.org:8080/#/c/23709/2/be/src/exprs/collection-functions-test.cc@128 PS2, Line 128: EXPECT_EQ(kCollectionSize, result3.val); : : ctx->impl()->Close(); > Can these asserts be removed since the value stored in `.val` has already b Done http://gerrit.cloudera.org:8080/#/c/23709/2/be/src/exprs/collection-functions.h File be/src/exprs/collection-functions.h: http://gerrit.cloudera.org:8080/#/c/23709/2/be/src/exprs/collection-functions.h@17 PS2, Line 17: : #pragma once : > Nit: use #pragma once Done http://gerrit.cloudera.org:8080/#/c/23709/2/fe/src/main/java/org/apache/impala/analysis/CollectionFunctionsBuiltins.java File fe/src/main/java/org/apache/impala/analysis/CollectionFunctionsBuiltins.java: http://gerrit.cloudera.org:8080/#/c/23709/2/fe/src/main/java/org/apache/impala/analysis/CollectionFunctionsBuiltins.java@43 PS2, Line 43: * Registers size() and length() functions for ARRAY and MAP types. : * Both functions return the number of elements in the collection. : */ : private static void registerSizeFunction > Nit: No need for these comments, please remove. Done http://gerrit.cloudera.org:8080/#/c/23709/2/fe/src/main/java/org/apache/impala/analysis/CollectionFunctionsBuiltins.java@80 PS2, Line 80: > Nit: these two need to their order switched. Done http://gerrit.cloudera.org:8080/#/c/23709/2/fe/src/main/java/org/apache/impala/util/FunctionUtils.java File fe/src/main/java/org/apache/impala/util/FunctionUtils.java: http://gerrit.cloudera.org:8080/#/c/23709/2/fe/src/main/java/org/apache/impala/util/FunctionUtils.java@174 PS2, Line 174: > Nit: no need for this portion of the comment, please remove. Done http://gerrit.cloudera.org:8080/#/c/23709/2/testdata/workloads/functional-query/queries/QueryTest/collection-functions.test File testdata/workloads/functional-query/queries/QueryTest/collection-functions.test: http://gerrit.cloudera.org:8080/#/c/23709/2/testdata/workloads/functional-query/queries/QueryTest/collection-functions.test@1 PS2, Line 1: ==== > Can tests be added to ensure size() works on nested arrays/maps (e.g. an ar Done http://gerrit.cloudera.org:8080/#/c/23709/2/testdata/workloads/functional-query/queries/QueryTest/collection-functions.test@77 PS2, Line 77: ---- QUERY : # Test length() with maps (alias for size) : select id, length(int_map) from complextypestbl order by id : ---- RESULTS : 1,2 : 2,2 : 3,0 : 4,0 : 5,0 : 6,NULL : 7,2 : 8,1 : ---- TYPES : bigint,int : ==== : ---- QUERY : # Test size() with array of maps : select id, size(int_map_array) from complextypestbl order by id : ---- RESULTS : 1,1 : 2,3 : 3,2 : 4,0 : 5,NULL : 6,NULL : 7,NULL : 8,4 : ---- TYPES : bigint,int : ==== : --- > These tests do not seem to be testing anything new. The previous tests cov Done http://gerrit.cloudera.org:8080/#/c/23709/2/testdata/workloads/functional-query/queries/QueryTest/collection-functions.test@175 PS2, Line 175: select id, length(int_array), length(int_map) from complextypestbl order by id : ---- RESULTS : 1,3,2 : 2,6,2 : 3,0,0 : 4,NULL,0 : 5,NULL,0 > This test does not seem to be testing anything new. The test on line 33 co Done http://gerrit.cloudera.org:8080/#/c/23709/2/testdata/workloads/functional-query/queries/QueryTest/collection-functions.test@183 PS2, Line 183: 7,NULL,2 : 8,1,1 : ---- TYPES : bigint,int,int : ==== : ---- QUERY : # Test size() with GROUP BY : select size(int_array) as arr_size, count(*) from complextypestbl group by size(int_array) order by arr_size nulls last : ---- RESULTS : 0,1 : 1,1 : 3,1 : 6,1 : NULL,4 > This test is asserting the case where the return from a UDF is used in the Done http://gerrit.cloudera.org:8080/#/c/23709/2/testdata/workloads/functional-query/queries/QueryTest/collection-functions.test@219 PS2, Line 219: 1,'NON-EMPTY' : 2,'NON-EMPTY' : 3,'EMPTY' : 4,'NULL' : 5,'NULL' : 6,'NULL' : 7,'NULL' : 8,'NON-EMPTY' : ---- TYPES : bigint,string > This test is asserting the case where the return from a UDF can be used in Done http://gerrit.cloudera.org:8080/#/c/23709/2/testdata/workloads/functional-query/queries/QueryTest/collection-functions.test@280 PS2, Line 280: select id from complextypestbl where size(int_array) is not null order by id : ---- RESULTS : 1 : 2 : 3 : 8 : ---- TYPES : bigint : ==== : ---- QUERY : # Test size() with COALESCE : select id, coalesce(size(int_array), -1) as arr_size from complextypestbl order by id : ---- RESULTS > Is this test asserting a new test case? Other tests already assert that si Done http://gerrit.cloudera.org:8080/#/c/23709/2/testdata/workloads/functional-query/queries/QueryTest/collection-functions.test@295 PS2, Line 295: 3,0 : 4,-1 : 5,-1 : 6,-1 : 7,-1 : 8,1 : ---- TYPES : bigint,int : ==== > This test is asserting the case where the return from a UDF can be used in Done -- To view, visit http://gerrit.cloudera.org:8080/23709 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I85b50386790657ea035addb35eed959d71810ab2 Gerrit-Change-Number: 23709 Gerrit-PatchSet: 4 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Sun, 05 Jul 2026 08:14:24 +0000 Gerrit-HasComments: Yes
