Skip to content

Commit 4eda93a

Browse files
abnegateclaude
abnegate
and
claude
committed
fix(sum): range-check a sum of grouped or distinct rows on mariadb and mysql
sum() over grouped or distinct rows adds up a column of the derived table as a raw SUM(), which the dialect builder never compiles, so it missed the `+ 0` that #1018 puts on every floating aggregate. MariaDB and MySQL would answer such a sum that overflowed a double with 0 instead of raising 1690. The column can be a dotted alias such as `r.score`, which the builder would split into two identifiers, so the adapter wraps the expression itself through rangeChecked(): unchanged on PostgreSQL and SQLite, which already raise or return INF for Database::sum() to refuse, and `+ 0` on MariaDB and MySQL. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 2e88fd0 commit 4eda93a

5 files changed

Lines changed: 181 additions & 1 deletion

File tree

‎src/Database/Adapter/MariaDB.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1174,6 +1174,12 @@ protected function dialectBuilder(): SQLBuilder&Scoping
11741174
return new MariaDBBuilder();
11751175
}
11761176

1177+
#[\Override]
1178+
protected function rangeChecked(string $expression): string
1179+
{
1180+
return $expression.' + 0';
1181+
}
1182+
11771183
#[\Override]
11781184
protected function boundsJoinedSort(): bool
11791185
{

‎src/Database/Adapter/SQL.php‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1952,7 +1952,7 @@ public function sum(Document $collection, string $attribute, array $queries = []
19521952

19531953
$outerBuilder = $this->dialectBuilder();
19541954
$outerBuilder->fromSub($this->rowsBuilder($collectionDoc, $plan, $name, $alias, $roles, $max), 'table_count');
1955-
$outerBuilder->selectRaw('SUM('.$this->quote($column).') AS '.$this->quote('sum'));
1955+
$outerBuilder->selectRaw($this->rangeChecked('SUM('.$this->quote($column).')').' AS '.$this->quote('sum'));
19561956

19571957
return $this->sumOf($this->fetchAggregateRow($outerBuilder, Event::DocumentSum, $name));
19581958
}
@@ -3457,6 +3457,15 @@ final public function builder(): SQLBuilder&Scoping
34573457
*/
34583458
abstract protected function dialectBuilder(): SQLBuilder&Scoping;
34593459

3460+
/**
3461+
* A floating expression the engine raises an error for when it overflows a double, rather than answer a wrong
3462+
* number. The dialect builder checks the aggregates it compiles the same way.
3463+
*/
3464+
protected function rangeChecked(string $expression): string
3465+
{
3466+
return $expression;
3467+
}
3468+
34603469
#[\Override]
34613470
public function schema(): MySQLSchema|PostgresSchema
34623471
{

‎tests/e2e/Adapter/Scopes/AggregationTests.php‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2952,6 +2952,48 @@ public function testSumAddsUpTheRowsOfGroupedAndDistinctReads(): void
29522952
}
29532953
}
29542954

2955+
public function testSumOfGroupedOrDistinctRowsThatOverflowsIsRefused(): void
2956+
{
2957+
$database = static::getDatabase();
2958+
if (! $database->getAdapter()->supports(Capability::Aggregations)) {
2959+
$this->expectNotToPerformAssertions();
2960+
2961+
return;
2962+
}
2963+
2964+
$collection = 'agg_rows_overflow'.$this->getAggSuffix();
2965+
$database->createCollection(Collection::create(
2966+
id: $collection,
2967+
attributes: [
2968+
Attribute::string(key: 'series', size: 20, required: true),
2969+
Attribute::double(key: 'reading', required: true),
2970+
],
2971+
permissions: [Permission::create(Role::any()), Permission::read(Role::any())],
2972+
));
2973+
foreach ([['huge', 1e308], ['huge', 1e308], ['huge', 9e307], ['sunk', -1e308], ['sunk', -9e307], ['single', 1e308]] as [$series, $reading]) {
2974+
$database->createDocument($collection, new Document([
2975+
'series' => $series,
2976+
'reading' => $reading,
2977+
'$permissions' => [Permission::read(Role::any())],
2978+
]));
2979+
}
2980+
2981+
foreach (['huge', 'sunk'] as $series) {
2982+
foreach ([[Query::count('*', 'n'), Query::groupBy(['reading'])], [Query::distinct(), Query::select(['reading'])]] as $shape) {
2983+
try {
2984+
$database->sum($collection, 'reading', [Query::equal('series', [$series]), ...$shape]);
2985+
$this->fail("A sum of {$series} grouped or distinct rows that overflows a double is refused");
2986+
} catch (LimitException) {
2987+
$this->addToAssertionCount(1);
2988+
}
2989+
}
2990+
}
2991+
2992+
$this->assertEqualsWithDelta(1e308, $database->sum($collection, 'reading', [Query::equal('series', ['single']), Query::distinct(), Query::select(['reading'])]), 1e293);
2993+
2994+
$database->deleteCollection($collection);
2995+
}
2996+
29552997
private function assertCountRefused(callable $count, string $label): void
29562998
{
29572999
$error = null;

‎tests/unit/Adapter/RowSumStatementTest.php‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
<?php
2+
3+
namespace Tests\Unit\Adapter;
4+
5+
use Closure;
6+
use PDO;
7+
use PDOStatement;
8+
use PHPUnit\Framework\Attributes\DataProvider;
9+
use PHPUnit\Framework\TestCase;
10+
use Utopia\Database\Adapter\MariaDB;
11+
use Utopia\Database\Adapter\MySQL;
12+
use Utopia\Database\Adapter\Postgres;
13+
use Utopia\Database\Adapter\SQL;
14+
use Utopia\Database\Attribute;
15+
use Utopia\Database\Collection;
16+
use Utopia\Database\Query;
17+
use Utopia\Database\Validator\Authorization;
18+
19+
/**
20+
* MariaDB and MySQL send a double sum that overflowed as `0`, so sum() over grouped or distinct rows adds them up
21+
* through the engine's range check, as it does over documents.
22+
*/
23+
final class RowSumStatementTest extends TestCase
24+
{
25+
/**
26+
* @return iterable<string, array{Closure(PDO): SQL, string}>
27+
*/
28+
public static function adapters(): iterable
29+
{
30+
yield 'mariadb' => [static fn (PDO $pdo): SQL => new MariaDB($pdo), 'SUM(`reading`) + 0 AS `sum`'];
31+
yield 'mysql' => [static fn (PDO $pdo): SQL => new MySQL($pdo), 'SUM(`reading`) + 0 AS `sum`'];
32+
yield 'postgres' => [static fn (PDO $pdo): SQL => new Postgres($pdo), 'SUM("reading") AS "sum"'];
33+
}
34+
35+
/**
36+
* @return iterable<string, array{list<Query>}>
37+
*/
38+
public static function shapes(): iterable
39+
{
40+
yield 'groupBy' => [[Query::count('*', 'n'), Query::groupBy(['reading'])]];
41+
yield 'distinct' => [[Query::distinct(), Query::select(['reading'])]];
42+
}
43+
44+
/**
45+
* @return iterable<string, array{Closure(PDO): SQL, string, list<Query>}>
46+
*/
47+
public static function sums(): iterable
48+
{
49+
foreach (self::adapters() as $adapter => [$create, $selection]) {
50+
foreach (self::shapes() as $shape => [$queries]) {
51+
yield "{$shape} on {$adapter}" => [$create, $selection, $queries];
52+
}
53+
}
54+
}
55+
56+
/**
57+
* @param Closure(PDO): SQL $create
58+
* @param list<Query> $queries
59+
*/
60+
#[DataProvider('sums')]
61+
public function testTheRowsAreAddedUpThroughTheRangeCheck(Closure $create, string $selection, array $queries): void
62+
{
63+
$statements = [];
64+
$pdo = $this->createStub(PDO::class);
65+
$pdo->method('prepare')->willReturnCallback(function (string $sql) use (&$statements): PDOStatement {
66+
$statements[] = $sql;
67+
$statement = $this->createStub(PDOStatement::class);
68+
$statement->method('execute')->willReturn(true);
69+
$statement->method('fetchAll')->willReturn([['sum' => '3']]);
70+
$statement->method('closeCursor')->willReturn(true);
71+
72+
return $statement;
73+
});
74+
75+
$adapter = $create($pdo);
76+
$adapter->setDatabase('database');
77+
$adapter->setNamespace('namespace');
78+
$adapter->setAuthorization(new Authorization());
79+
80+
$collection = Collection::create(id: 'readings', attributes: [Attribute::double(key: 'reading', required: true)]);
81+
82+
$this->assertSame(3, $adapter->sum($collection, 'reading', $queries));
83+
$this->assertCount(1, $statements);
84+
$this->assertStringStartsWith('SELECT '.$selection.' FROM (', $statements[0]);
85+
}
86+
}

‎tests/unit/SumOverflowTest.php‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,43 @@ public function testASumThatOverflowsUnderFiltersIsRefused(Closure $adapter): vo
9494
$database->sum(self::COLLECTION, 'reading', [Query::greaterThan('reading', 0)], 10);
9595
}
9696

97+
/**
98+
* @return iterable<string, array{Closure(): Adapter, list<Query>}>
99+
*/
100+
public static function rows(): iterable
101+
{
102+
foreach (['sqlite' => false, 'sqlite stringified' => true] as $label => $stringify) {
103+
$adapter = static fn (): Adapter => new SQLite(new PDO('sqlite::memory:', null, null, [PDO::ATTR_STRINGIFY_FETCHES => $stringify]));
104+
yield "groupBy on {$label}" => [$adapter, [Query::count('*', 'n'), Query::groupBy(['reading'])]];
105+
yield "distinct on {$label}" => [$adapter, [Query::distinct(), Query::select(['reading'])]];
106+
}
107+
}
108+
109+
/**
110+
* @param Closure(): Adapter $adapter
111+
* @param list<Query> $queries
112+
*/
113+
#[DataProvider('rows')]
114+
public function testASumOfGroupedOrDistinctRowsThatOverflowsIsRefused(Closure $adapter, array $queries): void
115+
{
116+
$database = $this->database($adapter, [1e308, 1e308, 9e307]);
117+
118+
$this->expectException(LimitException::class);
119+
$database->sum(self::COLLECTION, 'reading', $queries);
120+
}
121+
122+
/**
123+
* @param Closure(): Adapter $adapter
124+
* @param list<Query> $queries
125+
*/
126+
#[DataProvider('rows')]
127+
public function testTheLargestFiniteSumOfGroupedOrDistinctRowsIsStillAnswered(Closure $adapter, array $queries): void
128+
{
129+
$database = $this->database($adapter, [1e308, 1e308]);
130+
131+
$this->assertSame(1e308, $database->sum(self::COLLECTION, 'reading', $queries));
132+
}
133+
97134
/**
98135
* @param Closure(): Adapter $adapter
99136
*/

0 commit comments

Comments
 (0)