Balazs Hevele 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 27: (11 comments) 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: set(tuple_mem, 0, num_rows * tupl > probably it is more efficient to memset the whole array to 0 Done http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@116 PS26, Line 116: return WriteSlotBatchImpl<true>(tuple_mem, tuple_size, start_row_idx, end_row_idx); > The arrow array could be also checked if it ha any null values: Done http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@194 PS26, Line 194: return SetDecimalVal(data, byte_length, slot) > Shouldn't the size match the size we expect exactly? The size should be cle Done http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@210 PS26, Line 210: > Arrow has its own decimal32/64/128/256 types. This would work, but we have arrow 15.0.0 which doesn't have decimal32 and 64 as they were added later. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@226 PS26, Line 226: Status WriteSlotInternal(int row_idx, void* slot) { > This doesn't change per row, the final Timezone ptr could be set in timezon Done http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@230 PS26, Line 230: ::arrow::TimeUnit::NANO: > This looks incorrect for negative values, because those are truncated towar TimestampValue::FromUnixTimeNanos does call SplitTime though, which truncates it towards negative infinity. There are tests in timestamp-test.cc which call FromUnixTimeNanos with a negative nanos value. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@246 PS26, Line 246: > This lacks validation (unlike Parquet). Or validation is done elsewhere? I added returning error status when encountering invalid values. For internally produced arrow arrays, it will make sense to add them in the patch that converts impala batches to arrow. http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@254 PS26, Line 254: > Not sure if the argument is useful just to pass this in for the error messa Done http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@267 PS26, Line 267: > If small enough then small strings could be created from the start. Done http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@268 PS26, Line 268: public SlotWriterImpl<StringSlotWriter<ArrowArrayType> > Would it be possible to point to the arrow array to avoid the copy? I assum Done http://gerrit.cloudera.org:8080/#/c/24190/26/be/src/exec/arrow-converter.cc@318 PS26, Line 318: break; : } : case TYPE_STRING: : case TYPE_VARCHAR: { : if (type.IsBinaryType()) { // byte[] : writer.reset(new StringSlotWriter<arrow::BinaryArray>(slot_desc, array)); : } else { : writer.reset(new StringSlotWriter<arrow::StringArray>(slot_desc, array)); : } : break; : } : case TYPE_BOOLEAN: { : writer.reset(new BaseSlotWriter<bool, arrow::BooleanArray>(slot_desc, array)); : break; : } > This looks strange - VARCHAR is written by StringSlotWriter, while CHAR is Done -- 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: 27 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: Tue, 28 Jul 2026 03:01:18 +0000 Gerrit-HasComments: Yes
