Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle more cases in the MergeDistributive search query optimizer #43975

Merged
merged 3 commits into from
Mar 5, 2024
Merged

Handle more cases in the MergeDistributive search query optimizer #43975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
277a6fc
test: Additional tests for issue #35776
starypatyk Feb 25, 2024
3971313
fix: don't short circuit query optimizer
icewind1991 Mar 4, 2024
6fb4476
fix: handle cases where non-binary operators are mixed in for optimiz…
icewind1991 Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 15 additions & 38 deletions lib/private/Files/Search/QueryOptimizer/MergeDistributiveOperations.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,19 @@
*/
class MergeDistributiveOperations extends ReplacingOptimizerStep {
public function processOperator(ISearchOperator &$operator): bool {
if (
$operator instanceof SearchBinaryOperator &&
$this->isAllSameBinaryOperation($operator->getArguments())
) {
if ($operator instanceof SearchBinaryOperator) {
// either 'AND' or 'OR'
$topLevelType = $operator->getType();

// split the arguments into groups that share a first argument
// (we already know that all arguments are binary operators with at least 1 child)
$groups = $this->groupBinaryOperatorsByChild($operator->getArguments(), 0);
$outerOperations = array_map(function (array $operators) use ($topLevelType) {
// no common operations, no need to change anything
if (count($operators) === 1) {
return $operators[0];
}

// for groups with size >1 we know they are binary operators with at least 1 child
/** @var ISearchBinaryOperator $firstArgument */
$firstArgument = $operators[0];

Expand Down Expand Up @@ -72,46 +70,25 @@ public function processOperator(ISearchOperator &$operator): bool {
return parent::processOperator($operator);
}

/**
* Check that a list of operators is all the same type of (non-empty) binary operators
*
* @param ISearchOperator[] $operators
* @return bool
* @psalm-assert-if-true SearchBinaryOperator[] $operators
*/
private function isAllSameBinaryOperation(array $operators): bool {
$operation = null;
foreach ($operators as $operator) {
if (!$operator instanceof SearchBinaryOperator) {
return false;
}
if (!$operator->getArguments()) {
return false;
}
if ($operation === null) {
$operation = $operator->getType();
} else {
if ($operation !== $operator->getType()) {
return false;
}
}
}
return true;
}

/**
* Group a list of binary search operators that have a common argument
*
* @param SearchBinaryOperator[] $operators
* @return SearchBinaryOperator[][]
* Non-binary operators, or empty binary operators will each get their own 1-sized group
*
* @param ISearchOperator[] $operators
* @return ISearchOperator[][]
*/
private function groupBinaryOperatorsByChild(array $operators, int $index = 0): array {
$result = [];
foreach ($operators as $operator) {
/** @var SearchBinaryOperator|SearchComparison $child */
$child = $operator->getArguments()[$index];
$childKey = (string) $child;
$result[$childKey][] = $operator;
if ($operator instanceof ISearchBinaryOperator && count($operator->getArguments()) > 0) {
/** @var SearchBinaryOperator|SearchComparison $child */
$child = $operator->getArguments()[$index];
$childKey = (string)$child;
$result[$childKey][] = $operator;
} else {
$result[] = [$operator];
}
}
return array_values($result);
}
Expand Down
4 changes: 3 additions & 1 deletion lib/private/Files/Search/QueryOptimizer/ReplacingOptimizerStep.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ public function processOperator(ISearchOperator &$operator): bool {
$modified = false;
$arguments = $operator->getArguments();
foreach ($arguments as &$argument) {
$modified = $modified || $this->processOperator($argument);
if ($this->processOperator($argument)) {
$modified = true;
}
}
if ($modified) {
$operator->setArguments($arguments);
Expand Down
146 changes: 146 additions & 0 deletions tests/lib/Files/Search/QueryOptimizer/CombinedTests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,150 @@ public function testBasicOrOfAnds() {

$this->assertEquals('(storage eq 1 and path in ["foo","bar","asd"])', $operator->__toString());
}

public function testComplexSearchPattern1() {
$operator = new SearchBinaryOperator(
ISearchBinaryOperator::OPERATOR_OR,
[
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 1),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 2),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "201"),
new SearchComparison(ISearchComparison::COMPARE_LIKE, "path", "201/%"),
]),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "301"),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 4),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "401"),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "302"),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 4),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "402"),
]),
]
);
$this->assertEquals('((storage eq 1) or (storage eq 2 and (path eq "201" or path like "201\/%")) or (storage eq 3 and path eq "301") or (storage eq 4 and path eq "401") or (storage eq 3 and path eq "302") or (storage eq 4 and path eq "402"))', $operator->__toString());

$this->optimizer->processOperator($operator);

$this->assertEquals('(storage eq 1 or (storage eq 2 and (path eq "201" or path like "201\/%")) or (storage eq 3 and path in ["301","302"]) or (storage eq 4 and path in ["401","402"]))', $operator->__toString());
}

public function testComplexSearchPattern2() {
$operator = new SearchBinaryOperator(
ISearchBinaryOperator::OPERATOR_OR,
[
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 1),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 2),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "201"),
new SearchComparison(ISearchComparison::COMPARE_LIKE, "path", "201/%"),
]),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "301"),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 4),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "401"),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "302"),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 4),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "402"),
]),
]
);
$this->assertEquals('(storage eq 1 or (storage eq 2 and (path eq "201" or path like "201\/%")) or (storage eq 3 and path eq "301") or (storage eq 4 and path eq "401") or (storage eq 3 and path eq "302") or (storage eq 4 and path eq "402"))', $operator->__toString());

$this->optimizer->processOperator($operator);

$this->assertEquals('(storage eq 1 or (storage eq 2 and (path eq "201" or path like "201\/%")) or (storage eq 3 and path in ["301","302"]) or (storage eq 4 and path in ["401","402"]))', $operator->__toString());
}

public function testApplySearchConstraints1() {
$operator = new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "mimetype", "image/png"),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "mimetype", "image/jpeg"),
]),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 1),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files"),
new SearchComparison(ISearchComparison::COMPARE_LIKE, "path", "files/%"),
]),
]),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 2),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files/301"),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files/302"),
]),
]),
]);
$this->assertEquals('(((mimetype eq "image\/png" or mimetype eq "image\/jpeg")) and ((storage eq 1 and (path eq "files" or path like "files\/%")) or storage eq 2 or (storage eq 3 and path eq "files\/301") or (storage eq 3 and path eq "files\/302")))', $operator->__toString());

$this->optimizer->processOperator($operator);

$this->assertEquals('(mimetype in ["image\/png","image\/jpeg"] and ((storage eq 1 and (path eq "files" or path like "files\/%")) or storage eq 2 or (storage eq 3 and path in ["files\/301","files\/302"])))', $operator->__toString());
}

public function testApplySearchConstraints2() {
$operator = new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "mimetype", "image/png"),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "mimetype", "image/jpeg"),
]),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 1),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files"),
new SearchComparison(ISearchComparison::COMPARE_LIKE, "path", "files/%"),
]),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 2),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files/301"),
]),
new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3),
new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files/302"),
]),
]),
]);
$this->assertEquals('(((mimetype eq "image\/png" or mimetype eq "image\/jpeg")) and ((storage eq 1 and (path eq "files" or path like "files\/%")) or (storage eq 2) or (storage eq 3 and path eq "files\/301") or (storage eq 3 and path eq "files\/302")))', $operator->__toString());

$this->optimizer->processOperator($operator);

$this->assertEquals('(mimetype in ["image\/png","image\/jpeg"] and ((storage eq 1 and (path eq "files" or path like "files\/%")) or storage eq 2 or (storage eq 3 and path in ["files\/301","files\/302"])))', $operator->__toString());
}
}
Loading