This is an automated email from the ASF dual-hosted git repository. csringhofer pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit b6ce98b0b60c12869b2cd88db481cbd16bbc21fe Author: Zoltan Borok-Nagy <[email protected]> AuthorDate: Thu Dec 14 18:33:50 2023 +0100 IMPALA-12606: Sporadic failures around query_test.test_queries.TestQueries.test_intersect test_intersect failed when ASYNC_CODEGEN was enabled. This happened because we were using codegened 'ProcessProbeBatch' in the HASH JOIN operator with non-codegened InsertBatch/ProcessBuildBatch at the Builder side, or vice versa. Only the NULL StringValues were hit by the bug, turned out NULLs are handled differently in the hash table. We are using the HashUtil::FNV_SEED number to represent NULL values. This number was chosen arbitrarily, we just wanted to use some random value. In the codegened path we invoke StringValue::Assign(ptr, len) with both params being HashUtil::FNV_SEED. HashUtil::FNV_SEED is a negative value in int32_t, so StringValue::Assign(ptr, len) stored 0 as len actually, and not HashUtil::FNV_SEED. This is needed to be resilient against invalid values in corrupt files. In non-codegened path we are creating a StringValue object by reinterpret casting a [HashUtil::FNV_SEED, HashUtil::FNV_SEED, ...] array to StringValue. Then in RawValue::WriteNonNullPrimitive() we invoke StringValue::Assign(StringValue&) that just memcopies the parameter to this. It cannot check for negative values, because the parameter StringValue might be a valid small string. To sum up, this is how a NULL string was represented in the HashTable: * Codegen path: ptr = HashUtil::FNV_SEED, len = 0 * Non-codegen path: ptr = HashUtil::FNV_SEED, len = HashUtil::FNV_SEED This is why the hash join operator was working incorrectly on NULL String values when some parts of it used Codegen'ed path while other parts were using the non-codegened path. To fix the issue, I introduced UnsafeAssign(ptr, len) which doesn't do any checks for 'ptr' or 'len', so we have the same StringValue for objects for NULLs at both the codegened and non-codegened paths. Testing: * Executed TestQueries.test_intersect multiple times Change-Id: I6b855c59808db80fd7ac596ce338fc4c3c9c7667 Reviewed-on: http://gerrit.cloudera.org:8080/20793 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- be/src/codegen/gen_ir_descriptions.py | 1 + be/src/exec/hash-table.cc | 6 +++--- be/src/runtime/smallable-string.h | 9 ++++++++- be/src/runtime/string-value-ir.cc | 4 ++++ be/src/runtime/string-value.h | 5 +++++ 5 files changed, 21 insertions(+), 4 deletions(-) diff --git a/be/src/codegen/gen_ir_descriptions.py b/be/src/codegen/gen_ir_descriptions.py index f8c573e01..62fda01fb 100755 --- a/be/src/codegen/gen_ir_descriptions.py +++ b/be/src/codegen/gen_ir_descriptions.py @@ -250,6 +250,7 @@ ir_functions = [ ["STRING_VALUE_LEN", "_ZNK6impala11StringValue5IrLenEv"], ["STRING_VALUE_SETLEN", "_ZN6impala11StringValue8IrSetLenEi"], ["STRING_VALUE_ASSIGN", "_ZN6impala11StringValue8IrAssignEPci"], + ["STRING_VALUE_UNSAFE_ASSIGN", "_ZN6impala11StringValue14IrUnsafeAssignEPci"], ["STRING_VALUE_CLEAR", "_ZN6impala11StringValue7IrClearEv"], ["BOOL_MIN_MAX_FILTER_ALWAYSTRUE", "_ZNK6impala16BoolMinMaxFilter10AlwaysTrueEv"], diff --git a/be/src/exec/hash-table.cc b/be/src/exec/hash-table.cc index 009196b20..c8576e335 100644 --- a/be/src/exec/hash-table.cc +++ b/be/src/exec/hash-table.cc @@ -712,9 +712,9 @@ static void CodegenAssignNullValue(LlvmCodeGen* codegen, LlvmBuilder* builder, if (type.type == TYPE_STRING || type.type == TYPE_VARCHAR) { llvm::Value* null_len = codegen->GetI32Constant(fnv_seed); llvm::Value* null_ptr = builder->CreateIntToPtr(null_len, codegen->ptr_type()); - llvm::Function* str_assign_fn = codegen->GetFunction( - IRFunction::STRING_VALUE_ASSIGN, false); - builder->CreateCall(str_assign_fn, + llvm::Function* str_unsafe_assign_fn = codegen->GetFunction( + IRFunction::STRING_VALUE_UNSAFE_ASSIGN, false); + builder->CreateCall(str_unsafe_assign_fn, llvm::ArrayRef<llvm::Value*>({dst, null_ptr, null_len})); } else { llvm::Value* null_value = NULL; diff --git a/be/src/runtime/smallable-string.h b/be/src/runtime/smallable-string.h index 5b8da44b5..24534e7c5 100644 --- a/be/src/runtime/smallable-string.h +++ b/be/src/runtime/smallable-string.h @@ -96,7 +96,8 @@ class __attribute__((__packed__)) SmallableString { memcpy(this, &other, sizeof(*this)); } - /// Assigns 'ptr' and 'len' to this string's long representation. + /// Assigns 'ptr' and 'len' to this string's long representation. Negative 'len' + /// is overwritten with 0. void Assign(char* ptr, int len) { // Invalid string values might have negative lengths. We also have backend tests // for this. @@ -106,6 +107,12 @@ class __attribute__((__packed__)) SmallableString { DCHECK(!IsSmall()); } + /// Assigns 'ptr' and 'len' to this string's long representation without any checks. + void UnsafeAssign(char* ptr, int len) { + rep.long_rep.ptr = ptr; + rep.long_rep.len = len; + } + void Clear() { memset(this, 0, sizeof(*this)); } bool IsSmall() const { diff --git a/be/src/runtime/string-value-ir.cc b/be/src/runtime/string-value-ir.cc index 0a5b82cb5..eb57bedee 100644 --- a/be/src/runtime/string-value-ir.cc +++ b/be/src/runtime/string-value-ir.cc @@ -30,6 +30,10 @@ IR_ALWAYS_INLINE void StringValue::IrAssign(char* ptr, int len) { Assign(ptr, len); } +IR_ALWAYS_INLINE void StringValue::IrUnsafeAssign(char* ptr, int len) { + UnsafeAssign(ptr, len); +} + IR_ALWAYS_INLINE void StringValue::IrClear() { Clear(); } } diff --git a/be/src/runtime/string-value.h b/be/src/runtime/string-value.h index 7f42cf145..ab9c840a9 100644 --- a/be/src/runtime/string-value.h +++ b/be/src/runtime/string-value.h @@ -70,6 +70,10 @@ public: string_impl_.Assign(ptr, len); } + void UnsafeAssign(char* ptr, int len) { + string_impl_.UnsafeAssign(ptr, len); + } + void Clear() { string_impl_.Clear(); } bool IsSmall() const { return string_impl_.IsSmall(); } @@ -93,6 +97,7 @@ public: char* IrPtr() const; void IrSetLen(int len); void IrAssign(char* ptr, int len); + void IrUnsafeAssign(char* ptr, int len); void IrClear(); // END IR FUNCTIONS
