You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by cs...@apache.org on 2023/12/19 14:07:27 UTC
(impala) 02/03: IMPALA-12606: Sporadic failures around query_test.test_queries.TestQueries.test_intersect
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 <bo...@cloudera.com>
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 <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
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