Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24190 )
Change subject: IMPALA-14882: part1: Convert arrow record batch to impala tuple batch ...................................................................... Patch Set 26: (12 comments) Still haven't processed the tests. Most comments are about timestamps - these look preexisting issues with timestamp handling in Paimon (which seems minimally tests) Probably they could be moved to a follow up ticket. http://gerrit.cloudera.org:8080/#/c/24190/24/be/src/exec/arrow-converter.cc File be/src/exec/arrow-converter.cc: http://gerrit.cloudera.org:8080/#/c/24190/24/be/src/exec/arrow-converter.cc@377 PS24, Line 377: > Arrow has a "timezone" type parameter for a Timestamp type, I haven't found This area needs some more comments - currently it is not explained what's actually happening. const std::string& tz_name = type.timezone(); const Timezone* tz = tz_name.empty() ? UTCPTR : timezone_; The tz_name non empty case fits timestamp with local timezone semantics, so the timestamp is interpreted as UTC, and converted into Impala's local timezone during scanning. The empty case means timezone-agnostic (in Arrow they use name "timestamp naive"), which means that we assume that the data is not written as offset in local timezone and requires no conversion to local timezone. If arrow conversion will be used internally in Impala and support round-trips between Impala's RowBatch and arrow, then timezone will need to be empty to avoid extra conversions. Setting timezone is only ok during scanning to convert from utc to local. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc File be/src/exec/arrow-converter.cc: http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@47 PS26, Line 47: uple->ClearNullBits(*tuple_desc); probably it is more efficient to memset the whole array to 0 http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@116 PS26, Line 116: if (slot_desc_->is_nullable()) { The arrow array could be also checked if it ha any null values: see HasValidityBitmap and MayHaveLogicalNulls in https://arrow.apache.org/docs/cpp/api/array.html http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@194 PS26, Line 194: DCHECK(byte_length > 0 && byte_length <= 16); Shouldn't the size match the size we expect exactly? The size should be clear from the Impala type used. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@210 PS26, Line 210: binary_array_ Arrow has its own decimal32/64/128/256 types. https://arrow.apache.org/docs/cpp/api/datatype.html I assume Paimon returns BinaryArray, but if we do internal conversions, it would be better to use these types. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@226 PS26, Line 226: const Timezone* tz = tz_name.empty() ? UTCPTR : timezone_; This doesn't change per row, the final Timezone ptr could be set in timezone_ in the constructor. It may also make sense to have have two different classes for Timestamps with or without timezone. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@230 PS26, Line 230: value / NANOS_PER_SEC, value % NANOS_PER_SEC This looks incorrect for negative values, because those are truncated towards zero. See UtcFromUnixTimeLimitedRangeNanos for how this is handled in Parquet. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@246 PS26, Line 246: return Status::OK(); This lacks validation (unlike Parquet). Or validation is done elsewhere? See ScalarColumnReader<TimestampValue, parquet::Type::INT96, true>::ValidateValue( TimestampValue* val) For validation probably the best would be to differentiate between internally and externally produced arrow arrays - for the external case we should validate if timestamps are in the valid range, while for internal case it should be a DCHECK. Based on Paimon docs they allow timestamps in range 0000-01-01 00:00:00.000000000 to 9999-12-31 23:59:59.999999999 - Impala only allows timestamps from 1400. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@254 PS26, Line 254: IS_BINARY Not sure if the argument is useful just to pass this in for the error message. The column name would be much more useful info. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@267 PS26, Line 267: v.size(); If small enough then small strings could be created from the start. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@268 PS26, Line 268: // Allocate memory and copy the bytes to the RowBatch. Would it be possible to point to the arrow array to avoid the copy? I assume that in the Paimon case the arrow version will be kept in memory while the row batch is processed. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@318 PS26, Line 318: case TYPE_CHAR: { : writer.reset(new VarCharSlotWriter(slot_desc, array, type.len)); : break; : } : case TYPE_STRING: : case TYPE_VARCHAR: { : if (type.IsBinaryType()) { // byte[] : writer.reset(new StringSlotWriter<true, arrow::BinaryArray>(slot_desc, array, : mem_pool)); : } else { : writer.reset(new StringSlotWriter<false, arrow::StringArray>(slot_desc, array, : mem_pool)); : } : break; : } This looks strange - VARCHAR is written by StringSlotWriter, while CHAR is written by VarCharSlotWriter. Shouldn't it be called CharSlotWriter? -- To view, visit http://gerrit.cloudera.org:8080/24190 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea544b3c71d9211c893f0fec3527ebe84155ebcd Gerrit-Change-Number: 24190 Gerrit-PatchSet: 26 Gerrit-Owner: Balazs Hevele <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Reviewer: jichen <[email protected]> Gerrit-Comment-Date: Wed, 22 Jul 2026 13:27:26 +0000 Gerrit-HasComments: Yes
