You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by wz...@apache.org on 2024/02/02 23:54:01 UTC
(impala) branch master updated (e489ab35b -> 3c93437b7)
This is an automated email from the ASF dual-hosted git repository.
wzhou pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
from e489ab35b IMPALA-12718: Provides UTF-8 support for the trim functions
new 61dd95369 IMPALA-12781: ARRAY<STRUCT<s: STRING> crashes in top-n
new 3c93437b7 IMPALA-12770: Fix infinite loop for nested Case expressions
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
be/src/runtime/descriptors.cc | 94 ++++++++++------------
be/src/runtime/descriptors.h | 27 +++++--
.../impala/rewrite/SimplifyConditionalsRule.java | 5 +-
.../impala/analysis/ExprRewriteRulesTest.java | 12 +++
.../queries/QueryTest/sort-complex.test | 9 +++
.../queries/QueryTest/top-n-complex.test | 9 +++
6 files changed, 98 insertions(+), 58 deletions(-)
(impala) 01/02: IMPALA-12781: ARRAY crashes in top-n
Posted by wz...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
wzhou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 61dd953691f1c4b83a8aa86d8f0e8a8031cd56a2
Author: Daniel Becker <da...@cloudera.com>
AuthorDate: Fri Feb 2 11:59:27 2024 +0100
IMPALA-12781: ARRAY<STRUCT<s: STRING> crashes in top-n
In ASAN builds, and if codegen is enabled, if we sort an array
containing a struct of a string with limit, Impala crashes.
This is because of an error in
SlotDescriptor::CodegenWriteCollectionStructChild(). This function is
responsible for copying the variable length data of struct children, but
currently we return early without doing anything if
'children_tuple_desc->string_slots()' and
'children_tuple_desc->collection_slots()' are empty. However, this is
incorrect because the variable length children of a struct are listed in
the corresponding members of the struct's parent slot, not the struct
slot itself; see the comment in TupleDescriptor::AddSlot(). Because of
this, the variable length data of the StringValue is not copied.
This commit removes the early return from
SlotDescriptor::CodegenWriteCollectionStructChild() and inserts a DCHECK
in TupleDescriptor::string_slots() and
TupleDescriptor::collection_slots() that ensures that these functions
are not called on item tuples of structs, as the result would be
unexpected/incorrect.
This change also adjusts some variable names in
SlotDescriptor::CodegenWriteCollectionToSlot() to make them clearer and
updates the codegen IR example of
SlotDescriptor::CodegenWriteStringOrCollectionToSlot().
Testing:
- added a query in top-n-complex.test that used to crash, and the same
query without LIMIT in sort-complex.test for completeness.
Change-Id: If87d9e44775e809da9a13e953b4d2c3db9728801
Reviewed-on: http://gerrit.cloudera.org:8080/20988
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/runtime/descriptors.cc | 94 ++++++++++------------
be/src/runtime/descriptors.h | 27 +++++--
.../queries/QueryTest/sort-complex.test | 9 +++
.../queries/QueryTest/top-n-complex.test | 9 +++
4 files changed, 82 insertions(+), 57 deletions(-)
diff --git a/be/src/runtime/descriptors.cc b/be/src/runtime/descriptors.cc
index 1d472c0df..4d98eeffa 100644
--- a/be/src/runtime/descriptors.cc
+++ b/be/src/runtime/descriptors.cc
@@ -1035,8 +1035,8 @@ void SlotDescriptor::CodegenStoreNonNullAnyVal(
switch (type.type) {
case TYPE_STRING:
case TYPE_VARCHAR:
- case TYPE_ARRAY: // CollectionVal has same memory layout as StringVal.
- case TYPE_MAP: { // CollectionVal has same memory layout as StringVal.
+ case TYPE_ARRAY:
+ case TYPE_MAP: {
CodegenWriteStringOrCollectionToSlot(read_write_info, raw_val_ptr,
pool_val, slot_desc, insert_before);
break;
@@ -1155,29 +1155,29 @@ void SlotDescriptor::CodegenSetToNull(const CodegenAnyValReadWriteInfo& read_wri
// br i1 %is_null7, label %null6, label %non_null5
//
// non_null5: ; preds = %entry4
+// %src8 = extractvalue { i64, i8* } %src3, 1
// %6 = extractvalue { i64, i8* } %src3, 0
// %7 = ashr i64 %6, 32
// %8 = trunc i64 %7 to i32
-// %src8 = extractvalue { i64, i8* } %src3, 1
// %slot10 = getelementptr inbounds <{ %"struct.impala::CollectionValue", i32, i8 }>,
// <{ %"struct.impala::CollectionValue", i32, i8 }>* %tuple, i32 0, i32 0
// %9 = insertvalue %"struct.impala::CollectionValue" zeroinitializer, i32 %8, 1
-// %10 = mul i32 %8, 13
-// %11 = sext i32 %10 to i64
+// %coll_tuple_byte_len = mul i32 %8, 13
+// %10 = sext i32 %coll_tuple_byte_len to i64
// %new_coll_val_ptr = call i8* @_ZN6impala7MemPool8AllocateILb0EEEPhli(
-// %"class.impala::MemPool"* %pool, i64 %11, i32 8)
-// call void @llvm.memcpy.p0i8.p0i8.i32(i8* %new_coll_val_ptr, i8* %src8, i32 %10,
-// i32 0, i1 false)
-// %12 = insertvalue %"struct.impala::CollectionValue" %9, i8* %new_coll_val_ptr, 0
+// %"class.impala::MemPool"* %pool, i64 %10, i32 8)
+// call void @llvm.memcpy.p0i8.p0i8.i32(i8* %new_coll_val_ptr, i8* %src8,
+// i32 %coll_tuple_byte_len, i32 0, i1 false)
+// %11 = insertvalue %"struct.impala::CollectionValue" %9, i8* %new_coll_val_ptr, 0
// store i32 0, i32* %item_index_addr
// br label %loop_condition_block
//
// null6: ; preds = %entry4
-// %13 = bitcast <{ %"struct.impala::CollectionValue", i32, i8 }>* %tuple to i8*
-// %null_byte_ptr15 = getelementptr inbounds i8, i8* %13, i32 16
-// %null_byte16 = load i8, i8* %null_byte_ptr15
-// %null_bit_set17 = or i8 %null_byte16, 1
-// store i8 %null_bit_set17, i8* %null_byte_ptr15
+// %12 = bitcast <{ %"struct.impala::CollectionValue", i32, i8 }>* %tuple to i8*
+// %null_byte_ptr14 = getelementptr inbounds i8, i8* %12, i32 16
+// %null_byte15 = load i8, i8* %null_byte_ptr14
+// %null_bit_set16 = or i8 %null_byte15, 1
+// store i8 %null_bit_set16, i8* %null_byte_ptr14
// br label %end_write9
//
// loop_condition_block: ; preds = %loop_increment_block, %non_null5
@@ -1187,11 +1187,11 @@ void SlotDescriptor::CodegenSetToNull(const CodegenAnyValReadWriteInfo& read_wri
//
// loop_body_block: ; preds = %loop_condition_block
// %children_tuple_array = bitcast i8* %new_coll_val_ptr
-// to <{ %"struct.impala::StringValue", i8 }>*
-// %children_tuple = getelementptr inbounds <{ %"struct.impala::StringValue", i8 }>,
-// <{ %"struct.impala::StringValue", i8 }>* %children_tuple_array, i32 %item_index
-// %14 = bitcast <{ %"struct.impala::StringValue", i8 }>* %children_tuple to i8*
-// %null_byte_ptr11 = getelementptr inbounds i8, i8* %14, i32 12
+// to <{ %"class.impala::StringValue", i8 }>*
+// %children_tuple = getelementptr inbounds <{ %"class.impala::StringValue", i8 }>,
+// <{ %"class.impala::StringValue", i8 }>* %children_tuple_array, i32 %item_index
+// %13 = bitcast <{ %"class.impala::StringValue", i8 }>* %children_tuple to i8*
+// %null_byte_ptr11 = getelementptr inbounds i8, i8* %13, i32 12
// %null_byte12 = load i8, i8* %null_byte_ptr11
// %null_mask = and i8 %null_byte12, 1
// %is_null13 = icmp ne i8 %null_mask, 0
@@ -1204,31 +1204,26 @@ void SlotDescriptor::CodegenSetToNull(const CodegenAnyValReadWriteInfo& read_wri
// br label %loop_condition_block
//
// loop_exit_block: ; preds = %loop_condition_block
-// store %"struct.impala::CollectionValue" %12,
+// store %"struct.impala::CollectionValue" %11,
// %"struct.impala::CollectionValue"* %slot10
// br label %end_write9
//
// child_non_null_block: ; preds = %loop_body_block
// %child_str_or_coll_value_addr = getelementptr inbounds
-// <{ %"struct.impala::StringValue", i8 }>,
-// <{ %"struct.impala::StringValue", i8 }>* %children_tuple, i32 0, i32 0
-// %child_str_or_coll_value = load %"struct.impala::StringValue",
-// %"struct.impala::StringValue"* %child_str_or_coll_value_addr
-// %child_str_or_coll_value_ptr = extractvalue
-// %"struct.impala::StringValue" %child_str_or_coll_value, 0
-// %child_str_or_coll_value_len = extractvalue
-// %"struct.impala::StringValue" %child_str_or_coll_value, 1
-// %15 = insertvalue %"struct.impala::StringValue" zeroinitializer,
-// i32 %child_str_or_coll_value_len, 1
-// %16 = sext i32 %child_str_or_coll_value_len to i64
-// %new_coll_val_ptr14 = call i8* @_ZN6impala7MemPool8AllocateILb0EEEPhli(
-// %"class.impala::MemPool"* %pool, i64 %16, i32 8)
-// call void @llvm.memcpy.p0i8.p0i8.i32(i8* %new_coll_val_ptr14,
-// i8* %child_str_or_coll_value_ptr, i32 %child_str_or_coll_value_len, i32 0,
-// i1 false)
-// %17 = insertvalue %"struct.impala::StringValue" %15, i8* %new_coll_val_ptr14, 0
-// store %"struct.impala::StringValue" %17,
-// %"struct.impala::StringValue"* %child_str_or_coll_value_addr
+// <{ %"class.impala::StringValue", i8 }>,
+// <{ %"class.impala::StringValue", i8 }>* %children_tuple, i32 0, i32 0
+// %child_str_or_coll_value_ptr = call i8* @_ZNK6impala11StringValue5IrPtrEv(
+// %"class.impala::StringValue"* %child_str_or_coll_value_addr)
+// %child_str_or_coll_value_len = call i32 @_ZNK6impala11StringValue5IrLenEv(
+// %"class.impala::StringValue"* %child_str_or_coll_value_addr)
+// %14 = sext i32 %child_str_or_coll_value_len to i64
+// %new_ptr = call i8* @_ZN6impala7MemPool8AllocateILb0EEEPhli(
+// %"class.impala::MemPool"* %pool, i64 %14, i32 8)
+// call void @llvm.memcpy.p0i8.p0i8.i32(i8* %new_ptr, i8* %child_str_or_coll_value_ptr,
+// i32 %child_str_or_coll_value_len, i32 0, i1 false)
+// call void @_ZN6impala11StringValue8IrAssignEPci(
+// %"class.impala::StringValue"* %child_str_or_coll_value_addr, i8* %new_ptr,
+// i32 %child_str_or_coll_value_len)
// br label %next_block_after_child_is_written
//
// next_block_after_child_is_written: ; preds = %child_non_null_block, %loop_body_block
@@ -1318,19 +1313,21 @@ void SlotDescriptor::CodegenWriteCollectionToSlot(
coll_value = CodegenCollValueSetLen(codegen, builder, coll_value,
read_write_info.GetPtrAndLen().len);
if (pool_val != nullptr) {
- llvm::Value* len = read_write_info.GetPtrAndLen().len;
+ llvm::Value* num_tuples = read_write_info.GetPtrAndLen().len;
DCHECK(slot_desc != nullptr) << "SlotDescriptor needed to calculate the size of "
<< "the collection for copying.";
// For a 'CollectionValue', 'len' is not the byte size of the whole data but the
// number of items, so we have to multiply it with the byte size of the item tuple
// to get the data size.
int item_tuple_byte_size = slot_desc->children_tuple_descriptor()->byte_size();
- len = builder->CreateMul(len, codegen->GetI32Constant(item_tuple_byte_size));
+ llvm::Value* byte_len = builder->CreateMul(num_tuples,
+ codegen->GetI32Constant(item_tuple_byte_size), "coll_tuple_byte_len");
// Allocate a 'new_ptr' from 'pool_val' and copy the data from 'read_write_info->ptr'.
llvm::Value* new_ptr = codegen->CodegenMemPoolAllocate(
- builder, pool_val, len, "new_coll_val_ptr");
- codegen->CodegenMemcpy(builder, new_ptr, read_write_info.GetPtrAndLen().ptr, len);
+ builder, pool_val, byte_len, "new_coll_val_ptr");
+ codegen->CodegenMemcpy(builder, new_ptr, read_write_info.GetPtrAndLen().ptr,
+ byte_len);
coll_value = CodegenCollValueSetPtr(codegen, builder, coll_value, new_ptr);
slot_desc->CodegenWriteCollectionItemsToSlot(codegen, builder, new_ptr,
@@ -1449,7 +1446,7 @@ void SlotDescriptor::CodegenWriteCollectionIterateOverChildren(LlvmCodeGen* code
child_slot_desc->CodegenWriteCollectionVarlenChild(codegen, builder, children_tuple,
fn, insert_before, pool_val);
} else if (child_type.IsStructType()) {
- child_slot_desc-> CodegenWriteCollectionStructChild(codegen, builder,
+ child_slot_desc->CodegenWriteCollectionStructChild(codegen, builder,
children_tuple, fn, insert_before, pool_val);
}
}
@@ -1463,16 +1460,11 @@ void SlotDescriptor::CodegenWriteCollectionStructChild(LlvmCodeGen* codegen,
const TupleDescriptor* children_tuple_desc = children_tuple_descriptor();
DCHECK(children_tuple_desc != nullptr);
- // We only need to handle var-len descendant slots.
- if (children_tuple_desc->string_slots().empty()
- && children_tuple_desc->collection_slots().empty()) {
- return;
- }
-
llvm::Value* children_tuple = builder->CreateStructGEP(nullptr, tuple,
llvm_field_idx(), "struct_children_tuple");
- CodegenWriteCollectionIterateOverChildren(codegen, builder, children_tuple ,fn,
+ // TODO IMPALA-12775: Check whether the struct itself is NULL.
+ CodegenWriteCollectionIterateOverChildren(codegen, builder, children_tuple, fn,
insert_before, pool_val);
}
diff --git a/be/src/runtime/descriptors.h b/be/src/runtime/descriptors.h
index c3f5058f1..b53b50a6b 100644
--- a/be/src/runtime/descriptors.h
+++ b/be/src/runtime/descriptors.h
@@ -582,11 +582,22 @@ class TupleDescriptor {
int IR_NO_INLINE num_null_bytes() const { return num_null_bytes_; }
int IR_NO_INLINE null_bytes_offset() const { return null_bytes_offset_; }
const std::vector<SlotDescriptor*>& slots() const { return slots_; }
- const std::vector<SlotDescriptor*>& string_slots() const { return string_slots_; }
+
+ const std::vector<SlotDescriptor*>& string_slots() const {
+ DCHECK(!isTupleOfStructSlot());
+ return string_slots_;
+ }
+
const std::vector<SlotDescriptor*>& collection_slots() const {
+ DCHECK(!isTupleOfStructSlot());
return collection_slots_;
}
- bool HasVarlenSlots() const { return has_varlen_slots_; }
+
+ bool HasVarlenSlots() const {
+ DCHECK(!isTupleOfStructSlot());
+ return has_varlen_slots_;
+ }
+
const SchemaPath& tuple_path() const { return tuple_path_; }
const TableDescriptor* table_desc() const { return table_desc_; }
@@ -629,14 +640,18 @@ class TupleDescriptor {
/// them. See Tuple::MaterializeExprs().
std::vector<SlotDescriptor*> slots_;
- /// Contains only materialized string slots.
+ /// Contains the materialized string slots of this TupleDescriptor and also those of its
+ /// struct children (recursively). Not valid for item tuple descriptors of structs.
std::vector<SlotDescriptor*> string_slots_;
- /// Contains only materialized map and array slots.
+ /// Contains the materialized map and array slots of this TupleDescriptor and also those
+ /// of its struct children (recursively). Not valid for item tuple descriptors of
+ /// structs.
std::vector<SlotDescriptor*> collection_slots_;
- /// Provide quick way to check if there are variable length slots.
- /// True if string_slots_ or collection_slots_ have entries.
+ /// Provides a quick way to check if there are variable length slots in this tuple or
+ /// its struct children (recursively), i.e. true if string_slots_ or collection_slots_
+ /// has entries. Not valid for item tuple descriptors of structs.
bool has_varlen_slots_;
/// Absolute path into the table schema pointing to the collection whose fields are
diff --git a/testdata/workloads/functional-query/queries/QueryTest/sort-complex.test b/testdata/workloads/functional-query/queries/QueryTest/sort-complex.test
index ee1d25b2a..eaf8f858c 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/sort-complex.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/sort-complex.test
@@ -142,6 +142,15 @@ select id, arr_contains_struct, arr_contains_nested_struct from collection_struc
INT,STRING,STRING
====
---- QUERY
+# Sorting a var-len struct nested in a collection.
+select id, arr_contains_nested_struct from collection_struct_mix order by id;
+---- RESULTS
+1,'[{"inner_struct":{"str":"","l":0},"small":2},null,{"inner_struct":{"str":"some spaceship captain","l":5},"small":20}]'
+2,'[{"inner_struct":null,"small":104},{"inner_struct":{"str":"a few soju distilleries","l":28},"small":105},null]'
+---- TYPES
+INT,STRING
+====
+---- QUERY
# Sorting is not supported yet for collections within structs: IMPALA-12160. Test with a
# struct that contains an array.
select id, struct_contains_arr from collection_struct_mix order by id
diff --git a/testdata/workloads/functional-query/queries/QueryTest/top-n-complex.test b/testdata/workloads/functional-query/queries/QueryTest/top-n-complex.test
index f55b5ae7a..53b639818 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/top-n-complex.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/top-n-complex.test
@@ -139,6 +139,15 @@ select id, arr_contains_struct, arr_contains_nested_struct from collection_struc
INT,STRING,STRING
====
---- QUERY
+# Sorting a var-len struct nested in a collection. Regression test for IMPALA-12781.
+select id, arr_contains_nested_struct from collection_struct_mix order by id limit 2;
+---- RESULTS
+1,'[{"inner_struct":{"str":"","l":0},"small":2},null,{"inner_struct":{"str":"some spaceship captain","l":5},"small":20}]'
+2,'[{"inner_struct":null,"small":104},{"inner_struct":{"str":"a few soju distilleries","l":28},"small":105},null]'
+---- TYPES
+INT,STRING
+====
+---- QUERY
# Sorting is not supported yet for collections within structs: IMPALA-12160. Test with a
# struct that contains an array.
select id, struct_contains_arr from collection_struct_mix order by id limit 5
(impala) 02/02: IMPALA-12770: Fix infinite loop for nested Case expressions
Posted by wz...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
wzhou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 3c93437b7a6191a9ca1ef3acc740fefd2ae54bcf
Author: wzhou-code <wz...@cloudera.com>
AuthorDate: Thu Feb 1 15:09:29 2024 -0800
IMPALA-12770: Fix infinite loop for nested Case expressions
ExprRewriter enter infinite loop when run following query with
nested Case expressions:
select
case
case ''
when 'abc' then t4.string_col
end
when 'none' then 'Total'
end
as fcol from functional.alltypes as t4 limit 1;
The 'case' expr is set as literal NULL expression for outer case
expression since all 'when' expr are false in inner case expression.
This causes function SimplifyConditionalsRule.simplifyCaseExpr() to
be called recusively and enter infinite loop.
This patch fixes the issue by not rewriting a case expression if 'case'
expr is literal NULL expression.
Testing:
- Manually ran above query successfully.
- Added new test cases in ExprRewriteRulesTest.testCaseWithExpr.
- Passed core-tests.
Change-Id: Iaea1b3a35967f92b3e4f3445a378ed58adaa7da5
Reviewed-on: http://gerrit.cloudera.org:8080/20982
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
.../org/apache/impala/rewrite/SimplifyConditionalsRule.java | 5 ++++-
.../org/apache/impala/analysis/ExprRewriteRulesTest.java | 12 ++++++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java b/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
index 33d50cf83..15c752520 100644
--- a/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
+++ b/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
@@ -208,7 +208,10 @@ public class SimplifyConditionalsRule implements ExprRewriteRule {
private Expr simplifyCaseExpr(CaseExpr expr, Analyzer analyzer)
throws AnalysisException {
Expr caseExpr = expr.hasCaseExpr() ? expr.getChild(0) : null;
- if (expr.hasCaseExpr() && !Expr.IS_LITERAL.apply(caseExpr)) return expr;
+ if (expr.hasCaseExpr() &&
+ (!Expr.IS_LITERAL.apply(caseExpr) || Expr.IS_NULL_LITERAL.apply(caseExpr))) {
+ return expr;
+ }
int numChildren = expr.getChildren().size();
int loopStart = expr.hasCaseExpr() ? 1 : 0;
diff --git a/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
index dd38aface..2fe863434 100644
--- a/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
@@ -475,6 +475,18 @@ public class ExprRewriteRulesTest extends FrontendTestBase {
RewritesOk("case 0 when null then id else 1 end", rule, "1");
// All non-constant, don't rewrite.
RewritesOk("case id when 1 then 1 when 2 then 2 else 3 end", rule, null);
+ // IMPALA-12770: Fix infinite loop for nested Case expressions.
+ // Case NULL, don't rewrite.
+ RewritesOk("case NULL when id then id else 1 end", rule, null);
+ // Nested Case expressions.
+ RewritesOk("case case 1 when 0 then 0 when 1 then id end "
+ + "when 2 then id + 2 else id + 3 end",
+ rule, "CASE id WHEN 2 THEN id + 2 ELSE id + 3 END");
+ // 'when' are all FALSE for inner Case expression, set 'case' expr as NULL for outer
+ // Case expression.
+ RewritesOk("case case 3 when 0 then id when 1 then id + 1 end "
+ + "when 2 then id + 2 end",
+ rule, "CASE NULL WHEN 2 THEN id + 2 END");
}
@Test