8000 Fix crash in optimizer rule remove-collect-variables (#14824) · arangodb/arangodb@eec3f1c · GitHub 10000
[go: up one dir, main page]
Skip to content

Commit eec3f1c

Browse files
goedderzgoedderz
authored
Fix crash in optimizer rule remove-collect-variables (#14824)
1 parent d4bea91 commit eec3f1c

File tree

9 files changed

+81
-224
lines changed

9 files changed

+81
-224
lines changed

‎CHANGELOG‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,15 @@
11
devel
22
-----
33

4+
* Fix issue #14807: Fix crash during optimization of certain AQL queries during
5+
the remove-collect-variables optimizer rule, when a COLLECT node without output
6+
variables (this includes RETURN DISTINCT) occured in the plan.
7+
48
* Update iresearch library to the upstream. Fixed TSan/ASan detected issues.
59

610
* Added new ArangoSearch analyzer type 'collation'
711

12+
813
* Add basic overload control to arangod.
914
This change adds the `x-arango-queue-time-seconds` header to all responses
1015
sent by arangod. This header contains the most recent request dequeing time

‎arangod/Aql/Ast.cpp‎

Lines changed: 0 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -2465,91 +2465,6 @@ TopLevelAttributes Ast::getReferencedAttributes(AstNode const* node, bool& isSaf
24652465
return result;
24662466
}
24672467

2468-
/// @brief determines the to-be-kept attributes of an INTO expression
2469-
std::unordered_set<std::string> Ast::getReferencedAttributesForKeep(
2470-
AstNode const* node, Variable const* searchVariable, bool& isSafeForOptimization) {
2471-
auto isTargetVariable = [&searchVariable](AstNode const* node) {
2472-
if (node->type == NODE_TYPE_INDEXED_ACCESS) {
2473-
auto sub = node->getMemberUnchecked(0);
2474-
if (sub->type == NODE_TYPE_REFERENCE) {
2475-
Variable const* v = static_cast<Variable const*>(sub->getData());
2476-
if (v->id == searchVariable->id) {
2477-
return true;
2478-
}
2479-
}
2480-
} else if (node->type == NODE_TYPE_EXPANSION) {
2481-
if (node->numMembers() < 2) {
2482-
return false;
2483-
}
2484-
auto it = node->getMemberUnchecked(0);
2485-
if (it->type != NODE_TYPE_ITERATOR || it->numMembers() != 2) {
2486-
return false;
2487-
}
2488-
2489-
auto sub1 = it->getMember(0);
2490-
auto sub2 = it->getMember(1);
2491-
if (sub1->type != NODE_TYPE_VARIABLE || sub2->type != NODE_TYPE_REFERENCE) {
2492-
return false;
2493-
}
2494-
Variable const* v = static_cast<Variable const*>(sub2->getData());
2495-
if (v->id == searchVariable->id) {
2496-
return true;
2497-
}
2498-
}
2499-
2500-
return false;
2501-
};
2502-
2503-
std::unordered_set<std::string> result;
2504-
isSafeForOptimization = true;
2505-
2506-
auto visitor = [&isSafeForOptimization,
2507-
&result, &isTargetVariable,
2508-
&searchVariable](AstNode const* node) {
2509-
if (!isSafeForOptimization) {
2510-
return false;
2511-
}
2512-
2513-
if (node->type == NODE_TYPE_ATTRIBUTE_ACCESS) {
2514-
while (node->getMemberUnchecked(0)->type == NODE_TYPE_ATTRIBUTE_ACCESS) {
2515-
node = node->getMemberUnchecked(0);
2516-
}
2517-
if (isTargetVariable(node->getMemberUnchecked(0))) {
2518-
result.emplace(node->getString());
2519-
// do not descend further
2520-
return false;
2521-
}
2522-
} else if (node->type == NODE_TYPE_REFERENCE) {
2523-
Variable const* v = static_cast<Variable const*>(node->getData());
2524-
if (v->id == searchVariable->id) {
2525-
isSafeForOptimization = false;
2526-
return false;
2527-
}
2528-
} else if (node->type == NODE_TYPE_EXPANSION) {
2529-
if (isTargetVariable(node)) {
2530-
auto sub = node->getMemberUnchecked(1);
2531-
if (sub->type == NODE_TYPE_EXPANSION) {
2532-
sub = sub->getMemberUnchecked(0)->getMemberUnchecked(1);
2533-
}
2534-
if (sub->type == NODE_TYPE_ATTRIBUTE_ACCESS) {
2535-
while (sub->getMemberUnchecked(0)->type == NODE_TYPE_ATTRIBUTE_ACCESS) {
2536-
sub = sub->getMemberUnchecked(0);
2537-
}
2538-
result.emplace(sub->getString());
2539-
// do not descend further
2540-
return false;
2541-
}
2542-
}
2543-
}
2544-
2545-
return true;
2546-
};
2547-
2548-
traverseReadOnly(node, visitor, ::doNothingVisitor);
2549-
2550-
return result;
2551-
}
2552-
25532468
/// @brief determines the top-level attributes referenced in an expression for
25542469
/// the specified out variable
25552470
bool Ast::getReferencedAttributes(AstNode const* node, Variable const* variable,

‎arangod/Aql/Ast.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -450,9 +450,6 @@ class Ast {
450450
static bool getReferencedAttributesRecursive(AstNode const*, Variable const*,
451451
std::unordered_set<arangodb::aql::AttributeNamePath>&);
452452

453-
static std::unordered_set<std::string> getReferencedAttributesForKeep(
454-
AstNode const*, Variable const* searchVariable, bool&);
455-
456453
/// @brief replace an attribute access with just the variable
457454
static AstNode* replaceAttributeAccess(AstNode* node, Variable const* variable,
458455
std::vector<std::string> const& attributeName);

‎arangod/Aql/AstHelper.cpp‎

Lines changed: 18 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,11 @@ namespace {
3434

3535
auto doNothingVisitor = [](AstNode const*) {};
3636

37-
bool accessesSearchVariableViaReference(AstNode const* current, Variable const* searchVariable) {
37+
bool isTargetVariable(AstNode const* const current, Variable const* const searchVariable) {
3838
if (current->type == NODE_TYPE_INDEXED_ACCESS) {
3939
auto sub = current->getMemberUnchecked(0);
4040
if (sub->type == NODE_TYPE_REFERENCE) {
41-
Variable const* v = static_cast<Variable const*>(sub->getData());
41+
auto const v = static_cast<Variable const*>(sub->getData());
4242
if (v->id == searchVariable->id) {
4343
return true;
4444
}
@@ -57,7 +57,7 @@ bool accessesSearchVariableViaReference(AstNode const* current, Variable const*
5757
if (sub1->type != NODE_TYPE_VARIABLE || sub2->type != NODE_TYPE_REFERENCE) {
5858
return false;
5959
}
60-
Variable const* v = static_cast<Variable const*>(sub2->getData());
60+
auto const v = static_cast<Variable const*>(sub2->getData());
6161
if (v->id == searchVariable->id) {
6262
return true;
6363
}
@@ -66,113 +66,34 @@ bool accessesSearchVariableViaReference(AstNode const* current, Variable const*
6666
return false;
6767
};
6868

69-
bool isTargetVariable(AstNode const* node,
70-
::arangodb::containers::SmallVector<Variable const*>& searchVariables,
71-
bool& isSafeForOptimization) {
72-
TRI_ASSERT(!searchVariables.empty());
73-
74-
// given and expression like g3[0].`g2`[0].`g1`[0].`item1`.`_id`
75-
// this loop resolves subtrees of the form: .`g2`[0].`g1`[0]
76-
// search variables should equal [g1, g2, g3]. Therefor we need not match the
77-
// variable names from the vector with those in the expression while going
78-
// forward in the vector the we go backward in the expression
79-
80-
auto current = node;
81-
for (auto varIt = searchVariables.begin();
82-
varIt != std::prev(searchVariables.end()); ++varIt) {
83-
AstNode* next = nullptr;
84-
if (current->type == NODE_TYPE_INDEXED_ACCESS) {
85-
next = current->getMemberUnchecked(0);
86-
} else if (current->type == NODE_TYPE_EXPANSION) {
87-
if (current->numMembers() < 2) {
88-
return false;
89- }
90-
91-
auto it = current->getMemberUnchecked(0);
92-
TRI_ASSERT(it);
93-
94-
// The expansion is at the very end
95-
if (it->type == NODE_TYPE_ITERATOR && it->numMembers() == 2) {
96-
if (it->getMember(0)->type != NODE_TYPE_VARIABLE) {
97-
return false;
98-
}
99-
100-
auto attributeAccess = it->getMember(1);
101-
TRI_ASSERT(attributeAccess);
102-
103-
if (std::next(varIt) != searchVariables.end() &&
104-
attributeAccess->type == NODE_TYPE_ATTRIBUTE_ACCESS) {
105-
next = attributeAccess;
106-
} else {
107-
return false;
108-
}
109-
110-
} else {
111-
// The expansion is not at the very end! we are unable to check if the
112-
// variable will be accessed checking nested expansions is really crazy
113-
// and would probably be best done in a recursive way. If the expansion
114-
// is in the middle, then it would be still the very first node in the
115-
// AST, having 2 subtrees that contain the other search variables
116-
isSafeForOptimization = false; // could be an access - we can not tell
117-
return false;
118-
}
119-
} else {
120-
return false;
121-
}
122-
123-
if (varIt == searchVariables.end()) {
124-
return false;
125-
}
126-
127-
if (next && next->type == NODE_TYPE_ATTRIBUTE_ACCESS &&
128-
next->getString() == (*varIt)->name) {
129-
current = next->getMemberUnchecked(0);
130-
} else if (next && next->type == NODE_TYPE_EXPANSION) {
131-
isSafeForOptimization = false;
132-
return false;
133-
} else {
134-
return false;
135-
}
136-
} // for nodes but last
137-
138-
if (!current) {
139-
return false;
140-
}
141-
142-
return accessesSearchVariableViaReference(current, searchVariables.back());
143-
}
144-
14569
} // namespace
14670

147-
std::unordered_set<std::string> arangodb::aql::ast::getReferencedAttributesForKeep(
148-
const AstNode* node, ::arangodb::containers::SmallVector<const Variable*, 64> searchVariables,
149-
bool& isSafeForOptimization) {
150-
std::unordered_set<std::string> result;
71+
auto arangodb::aql::ast::getReferencedAttributesForKeep(AstNode const* const node,
72+
Variable const* const searchVariable,
73+
bool& isSafeForOptimization)
74+
-> std::vector<std::string> {
75+
auto result = std::vector<std::string>();
15176
isSafeForOptimization = true;
15277

153-
std::function<bool(AstNode const*)> visitor = [&isSafeForOptimization, &result,
154-
&searchVariables](AstNode const* node) {
155-
if (!isSafeForOptimization) {
156-
return false;
157-
}
158-
78+
auto const visitor = [&isSafeForOptimization, &result,
79+
&searchVariable](AstNode const* node) -> bool {
15980
if (node->type == NODE_TYPE_ATTRIBUTE_ACCESS) {
16081
while (node->getMemberUnchecked(0)->type == NODE_TYPE_ATTRIBUTE_ACCESS) {
16182
node = node->getMemberUnchecked(0);
16283
}
163-
if (isTargetVariable(node->getMemberUnchecked(0), searchVariables, isSafeForOptimization)) {
164-
result.emplace(node->getString());
84+
if (isTargetVariable(node->getMemberUnchecked(0), searchVariable)) {
85+
result.emplace_back(node->getString());
16586
// do not descend further
16687
return false;
16788
}
16889
} else if (node->type == NODE_TYPE_REFERENCE) {
169-
Variable const* v = static_cast<Variable const*>(node->getData());
170-
if (v->id == searchVariables.front()->id) {
90+
auto const v = static_cast<Variable const*>(node->getData());
91+
if (v->id == searchVariable->id) {
17192
isSafeForOptimization = false; // the expression references the searched variable
17293
return false;
17394
}
17495
} else if (node->type == NODE_TYPE_EXPANSION) {
175-
if (isTargetVariable(node, searchVariables, isSafeForOptimization)) {
96+
if (isTargetVariable(node, searchVariable)) {
17697
auto sub = node->getMemberUnchecked(1);
17798
if (sub->type == NODE_TYPE_EXPANSION) {
17899
sub = sub->getMemberUnchecked(0)->getMemberUnchecked(1);
@@ -181,16 +102,16 @@ std::unordered_set<std::string> arangodb::aql::ast::getReferencedAttributesForKe
181102
while (sub->getMemberUnchecked(0)->type == NODE_TYPE_ATTRIBUTE_ACCESS) {
182103
sub = sub->getMemberUnchecked(0);
183104
}
184-
result.emplace(sub->getString());
105+
result.emplace_back(sub->getString());
185106
// do not descend further
186107
return false;
187108
}
188109
}
189110
} else if (node->type == NODE_TYPE_INDEXED_ACCESS) {
190111
auto sub = node->getMemberUnchecked(0);
191112
if (sub- 85B0 >type == NODE_TYPE_REFERENCE) {
192-
Variable const* v = static_cast<Variable const*>(sub->getData());
193-
if (v->id == searchVariables.back()->id) {
113+
auto const v = static_cast<Variable const*>(sub->getData());
114+
if (v->id == searchVariable->id) {
194115
isSafeForOptimization = false;
195116
return false;
196117
}

‎arangod/Aql/AstHelper.h‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -24,22 +24,17 @@
2424
#pragma once
2525

2626
#include <string>
27-
#include <unordered_set>
27+
#include <vector>
2828

29-
#include "Containers/SmallVector.h"
30-
31-
namespace arangodb {
32-
namespace aql {
29+
namespace arangodb::aql {
3330
struct AstNode;
3431
struct Variable;
3532
namespace ast {
3633

3734
/// @brief determines the to-be-kept attribute of an INTO expression
38-
std::unordered_set<std::string> getReferencedAttributesForKeep(
39-
AstNode const* node, ::arangodb::containers::SmallVector<Variable const*> searchVariables,
40-
bool& isSafeForOptimization);
35+
auto getReferencedAttributesForKeep(AstNode const* node, const Variable* searchVariable,
36+
bool& isSafeForOptimization)
37+
-> std::vector<std::string>;
4138

4239
} // namespace ast
43-
} // namespace aql
44-
} // namespace arangodb
45-
40+
} // namespace arangodb::aql

‎arangod/Aql/CollectNode.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -780,7 +780,7 @@ std::vector<Variable const*> const& CollectNode::keepVariables() const {
780780
return _keepVariables;
781781
}
782782

783-
void CollectNode::restrictKeepVariables(std::unordered_set<const Variable*> const& variables) {
783+
void CollectNode::restrictKeepVariables(containers::HashSet<Variable const*> const& variables) {
784784
auto remainingKeepVariables = decltype(this->_keepVariables){};
785785
remainingKeepVariables.reserve(std::min(_keepVariables.size(), variables.size()));
786786

‎arangod/Aql/CollectNode.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ class CollectNode : public ExecutionNode {
155155

156156
/// @brief restrict the KEEP variables (which may also be the auto-collected
157157
/// variables of an unrestricted `INTO var`) to the passed `variables`.
158-
void restrictKeepVariables(std::unordered_set<const Variable*> const& variables);
158+
void restrictKeepVariables(containers::HashSet<Variable const*> const& variables);
159159

160160
/// @brief return the variable map
161161
std::unordered_map<VariableId, std::string const> const& variableMap() const;

0 commit comments

Comments
 (0)
0