Skip to content

Commit

Permalink
remove the expr::group feature, which caused a 20x slowdown in benchm…
Browse files Browse the repository at this point in the history
…ark - this change restores performance. the implications in terms of how many extra parens get added are actually quite reasonable - the conditional grouping feature seems to make very little difference on complex queries in practice. win/win :-)
  • Loading branch information
mindplay-dk committed Jun 2, 2024
1 parent 0b03dd7 commit d9574cc
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 29 deletions.
16 changes: 2 additions & 14 deletions src/model/expr.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public static function any(array $exprs): string
}

return count($exprs) > 1
? implode(" OR ", array_map(self::group(...), $exprs))
? "(" . implode(") OR (", $exprs) . ")"
: $exprs[0];
}

Expand All @@ -37,19 +37,7 @@ public static function all(array $exprs): string
}

return count($exprs) > 1
? implode(" AND ", array_map(self::group(...), $exprs))
? "(" . implode(") AND (", $exprs) . ")"
: $exprs[0];
}

private const FULLY_GROUPED = '/^\((?:[^()]*|\((?:[^()]*|\([^()]*\))*\))*\)$/';

/**
* Group an expression in parentheses, if not already fully grouped
*/
public static function group(string $expr): string
{
return preg_match(self::FULLY_GROUPED, $expr) === 1
? $expr
: "($expr)";
}
}
17 changes: 2 additions & 15 deletions test/test-unit.php
Original file line number Diff line number Diff line change
Expand Up @@ -888,19 +888,6 @@ function () {
}
);

test(
'can conditionally group SQL expressions',
function () {
eq(expr::group('a OR b'), '(a OR b)', 'should group');
eq(expr::group('(a) OR b'), '((a) OR b)', 'should group');
eq(expr::group('(a) OR (b)'), '((a) OR (b))', 'should group');

eq(expr::group('(a OR b)'), '(a OR b)', 'should not regroup');
eq(expr::group('((a) OR b)'), '((a) OR b)', 'should not regroup');
eq(expr::group('((a) OR (b))'), '((a) OR (b))', 'should not regroup');
}
);

test(
'can build and/or expressions',
function () {
Expand Down Expand Up @@ -930,8 +917,8 @@ function () {
'(a OR b)',
'(c OR d)'
]),
'(a OR b) AND (c OR d)',
'should not regroup nested expressions'
'((a OR b)) AND ((c OR d))',
'grouped expressions will be regrouped'
);

eq(
Expand Down

0 comments on commit d9574cc

Please sign in to comment.