Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/24062 )
Change subject: IMPALA-14116: Skip NULL in an IN-list against a column of an ORC table ...................................................................... Patch Set 9: (3 comments) I have addressed Quanlong's suggestions for patch set 8. Let me know if there are additional comments. Thanks! http://gerrit.cloudera.org:8080/#/c/24062/8/be/src/exec/orc/hdfs-orc-scanner.cc File be/src/exec/orc/hdfs-orc-scanner.cc: http://gerrit.cloudera.org:8080/#/c/24062/8/be/src/exec/orc/hdfs-orc-scanner.cc@1124 PS8, Line 1124: { > nit: pls remove extra space Done http://gerrit.cloudera.org:8080/#/c/24062/8/be/src/exec/orc/hdfs-orc-scanner.cc@1183 PS8, Line 1183: return orc::Literal(*predicate_type); > Let's fix this as well. Thanks for catching this! I missed it in this patch. I will correct this in the next patch. http://gerrit.cloudera.org:8080/#/c/24062/8/be/src/exec/orc/hdfs-orc-scanner.cc@1213 PS8, Line 1213: if (dst_ptr == nullptr) return orc::Literal(*predicate_type); > This also need a fix. Thanks for pointing this out too! I will change the instantiation to "orc::Literal(*predicate_type)" in the next patch. But I was also wondering if it is semantically correct to simply return "orc::Literal(*predicate_type)" if TryAllocate(dst_type.len) returns nullptr. /// Same as Allocate() except the mem limit is checked before the allocation and /// this call will fail (returns NULL) if it does. /// The caller must handle the NULL case. This should be used for allocations /// where the size can be very big to bound the amount by which we exceed mem limits. uint8_t* TryAllocate(int64_t size) noexcept { DFAKE_SCOPED_LOCK(mutex_); return Allocate<true>(size, DEFAULT_ALIGNMENT); } I am not familiar with the ORC library. My current understanding is that "orc::Literal(*predicate_type)" will return an instance representing a null literal in ORC. However, if we reach the line of "if (dst_ptr == nullptr) return orc::Literal(*predicate_type);", it implies 'val' above does not correspond to a NULL literal. It looks like we need additional logic to handle this case instead of returning "orc::Literal(*predicate_type)". if (UNLIKELY(!val)) return orc::Literal(*predicate_type); const StringValue* sv = reinterpret_cast<StringValue*>(val); StringValue::SimpleString s = sv->ToSimpleString(); char* dst_ptr; if (dst_type.len > s.len) { dst_ptr = reinterpret_cast<char*>(search_args_pool_->TryAllocate(dst_type.len)); if (dst_ptr == nullptr) return orc::Literal(*predicate_type); -- To view, visit http://gerrit.cloudera.org:8080/24062 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id62a631e5aa97132afbe0b184d427ad6bc1a4ad0 Gerrit-Change-Number: 24062 Gerrit-PatchSet: 9 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Mon, 23 Mar 2026 22:36:54 +0000 Gerrit-HasComments: Yes
