Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/17811 )
Change subject: POC IMPALA-9498: Allow returning arrays in select list ...................................................................... Patch Set 11: (7 comments) I looked through the C++ part quickly. http://gerrit.cloudera.org:8080/#/c/17811/11/be/src/exec/parquet/parquet-collection-column-reader.cc File be/src/exec/parquet/parquet-collection-column-reader.cc: http://gerrit.cloudera.org:8080/#/c/17811/11/be/src/exec/parquet/parquet-collection-column-reader.cc@58 PS11, Line 58: * Nit: we usually put the star on the type, not the variable name. http://gerrit.cloudera.org:8080/#/c/17811/11/be/src/runtime/raw-value.h File be/src/runtime/raw-value.h: http://gerrit.cloudera.org:8080/#/c/17811/11/be/src/runtime/raw-value.h@49 PS11, Line 49: /// Convert 'value' into ascii and write to 'stream'. If it only supports primitive types (not structs and arrays), it should be mentioned. http://gerrit.cloudera.org:8080/#/c/17811/11/be/src/runtime/raw-value.h@52 PS11, Line 52: /// If 'quote_val' is true, write STRING, VARCHAR, CHAR, DATE, TIMESTAMP values in : /// quoted form surrounded by double quotes. Shouldn't the other overloads of PrintValue also have this parameter for completeness? Or if we only use quoting within arrays, does it need to be a publicly exposed parameter? It could be a private overload with the public overload calling the private one with quote_val=false. Or could it be handled in 'PrintArrayValue' (though 'Utf8SafeCEscape' may be problematic..)? http://gerrit.cloudera.org:8080/#/c/17811/11/be/src/runtime/raw-value.cc File be/src/runtime/raw-value.cc: http://gerrit.cloudera.org:8080/#/c/17811/11/be/src/runtime/raw-value.cc@274 PS11, Line 274: //DCHECK(!slot_desc->type().IsComplexType()); Nit: should be deleted if not needed, or uncommented. http://gerrit.cloudera.org:8080/#/c/17811/11/be/src/runtime/raw-value.cc@434 PS11, Line 434: NULL Just an idea: wouldn't it be worth extracting the NULL string literals into a constant? http://gerrit.cloudera.org:8080/#/c/17811/11/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: http://gerrit.cloudera.org:8080/#/c/17811/11/be/src/service/impala-beeswax-server.cc@256 PS11, Line 256: complex If we support returning arrays as above, this should be changed to 'struct'. On the other hand, do we have to support arrays in Beeswax if we are going to deprecate it anyway? http://gerrit.cloudera.org:8080/#/c/17811/11/be/src/service/impala-beeswax-server.cc@623 PS11, Line 623: if (type.types.size() == 1) { : DCHECK_EQ(TTypeNodeType::SCALAR, type.types[0].type); : DCHECK(type.types[0].__isset.scalar_type); : TPrimitiveType::type col_type = type.types[0].scalar_type.type; : query_results->columns[i] = TypeToOdbcString(ThriftToType(col_type)); : } else if (type.types.size() > 1 && type.types[0].type == TTypeNodeType::ARRAY) { : // TODO: check this. Maybe we should simply call TypeToOdbcString() ? : // Also this code will have to be modified once other nested types are supported. : ostringstream arr_type_name; : for (int i = 0; i < type.types.size() - 1; ++i) { : DCHECK(type.types[i].type == TTypeNodeType::ARRAY); : arr_type_name << "list<"; : } : DCHECK(type.types.back().type == TTypeNodeType::SCALAR); : TPrimitiveType::type col_type = type.types.back().scalar_type.type; : arr_type_name << TypeToOdbcString(ThriftToType(col_type)); : arr_type_name << string('>', type.types.size() - 1); : query_results->columns[i] = arr_type_name.str(); : } If this code stays: if it is identical to the code starting on L234, it could be extracted to a function. -- To view, visit http://gerrit.cloudera.org:8080/17811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb1e42ffb21c7ddc033aba0f754b0108e46f34d0 Gerrit-Change-Number: 17811 Gerrit-PatchSet: 11 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Attila Jeges <[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-Comment-Date: Tue, 05 Oct 2021 13:10:54 +0000 Gerrit-HasComments: Yes
