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 24: (5 comments) High level comments, didn't look deeply into tests/benchmarks/paimon code. For optimization ideas feel free to postpone them to a later commit. 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@84 PS24, Line 84: if (arrow_array_->IsNull(row_idx)) { A potential optimization could be to optimize for cases with no NULLs to avoid this check per row. http://gerrit.cloudera.org:8080/#/c/24190/24/be/src/exec/arrow-converter.cc@89 PS24, Line 89: tuple->SetNotNull(null_indicator_offset); It may be faster to set all tuples to 0 (meaning non-null) and only set null bit in the NULL cases. Impala has template_tuple_ for this - set all tuples do some basic value (e.g. set partition columns), and the set remaining slots individually. http://gerrit.cloudera.org:8080/#/c/24190/24/be/src/exec/arrow-converter.cc@138 PS24, Line 138: template<class T, class AT> Can you add a comment about these types? Or a more descriptive name could help. I assume that it is Type and ArrowType, but needed some time. http://gerrit.cloudera.org:8080/#/c/24190/24/be/src/exec/arrow-converter.cc@214 PS24, Line 214: WriteSlotInternal Some of this code could be possibly merged with Parquet scanning code. ParquetPlainEncoder::Decode does similar work as some of these functions, so copies data with Parquet type to an Impala slot. Generally Iceberg types match Parquet plain encoded types, though null handling is different. For timestamps, see ParquetTimestampDecoder's Int64ToTimestampValue. http://gerrit.cloudera.org:8080/#/c/24190/24/be/src/exec/arrow-converter.cc@377 PS24, Line 377: local_time_zone Does arrow store timestamps as UTC? Iceberg uses mainly timestamps without time zone, which means that we assume that no timezone conversion is needed. -- 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: 24 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: Mon, 13 Jul 2026 10:15:19 +0000 Gerrit-HasComments: Yes
