fix(schema): address review feedback on ClickHouse skip indexes

lohanidamodar · lohanidamodar · commit 1f811df5f149 · 2026-04-30T00:57:14.000Z
Four follow-ups based on the greptile review on PR #6: - alter() now renders ADD INDEX for any skipIndexes set on the blueprint, matching ClickHouse's `ALTER TABLE ... ADD INDEX name expr TYPE algo GRANULARITY n` syntax. Previously dataSkippingIndex() inside an alter() callback was silently dropped (P1). - alter() throws UnsupportedException if the blueprint has settings — ClickHouse does not accept SETTINGS in ALTER TABLE; callers must emit `ALTER TABLE ... MODIFY SETTING` directly. Surfaces what was previously a silent no-op. - SkipIndex constructor rejects algorithmArgs for MinMax and Inverted (P2) — both are emitted without parentheses in ClickHouse DDL, so any args would have produced invalid SQL like `minmax(1)`. - Table::dataSkippingIndex() now sanitises auto-generated index names when columns contain non-identifier characters (P2) — `event-type` no longer produces a confusing `Invalid skip index name: skip_event-type` exception. Non-identifier characters are collapsed to `_`. - compileSkipAlgorithm() formats float args with sprintf('%F', ...) instead of (string) cast (P2) — the cast can produce scientific notation like `1.0E-5`, which ClickHouse rejects in index type arguments. Trailing zeros are trimmed for readability so 0.01 stays "0.01" rather than "0.010000". Adds tests for each fix plus one for ALTER ADD INDEX with composite columns and algorithm args.
diff --git a/src/Query/Schema/ClickHouse.php b/src/Query/Schema/ClickHouse.php
@@ -130,6 +130,14 @@ public function alter(string $table, callable $definition): Statement
             $alterations[] = 'DROP INDEX ' . $this->quote($name);
         }
 
+        foreach ($blueprint->skipIndexes as $skip) {
+            $cols = \array_map(fn (string $c): string => $this->quote($c), $skip->columns);
+            $expr = \count($cols) === 1 ? $cols[0] : '(' . \implode(', ', $cols) . ')';
+            $alterations[] = 'ADD INDEX ' . $this->quote($skip->name)
+                . ' ' . $expr . ' TYPE ' . $this->compileSkipAlgorithm($skip)
+                . ' GRANULARITY ' . $skip->granularity;
+        }
+
         if (! empty($blueprint->foreignKeys)) {
             throw new UnsupportedException('Foreign keys are not supported in ClickHouse.');
         }
@@ -138,6 +146,12 @@ public function alter(string $table, callable $definition): Statement
             throw new UnsupportedException('Foreign keys are not supported in ClickHouse.');
         }
 
+        if (! empty($blueprint->settings)) {
+            throw new UnsupportedException(
+                'Table SETTINGS can only be set on CREATE TABLE; emit `ALTER TABLE ... MODIFY SETTING` directly to change them.'
+            );
+        }
+
         if (empty($alterations)) {
             throw new ValidationException('ALTER TABLE requires at least one alteration.');
         }
@@ -245,9 +259,14 @@ private function compileSkipAlgorithm(SkipIndex $skip): string
         }
 
         $args = \array_map(
-            fn (string|int|float $arg): string => \is_string($arg)
-                ? "'" . \str_replace("'", "''", $arg) . "'"
-                : (string) $arg,
+            fn (string|int|float $arg): string => match (true) {
+                \is_string($arg) => "'" . \str_replace("'", "''", $arg) . "'",
+                // sprintf('%F', ...) avoids scientific notation (e.g. 1.0E-5)
+                // which ClickHouse rejects in index type arguments. Trim
+                // trailing zeros so 0.01 stays "0.010000" → "0.01".
+                \is_float($arg) => \rtrim(\rtrim(\sprintf('%F', $arg), '0'), '.'),
+                default => (string) $arg,
+            },
             $skip->algorithmArgs,
         );
 
diff --git a/src/Query/Schema/ClickHouse/SkipIndex.php b/src/Query/Schema/ClickHouse/SkipIndex.php
@@ -29,5 +29,23 @@ public function __construct(
         if ($granularity < 1) {
             throw new ValidationException('Skip index granularity must be >= 1.');
         }
+        if ($algorithmArgs !== [] && ! self::algorithmAcceptsArgs($algorithm)) {
+            throw new ValidationException(
+                $algorithm->value . ' does not accept algorithm arguments.'
+            );
+        }
+    }
+
+    /**
+     * MinMax and Inverted are emitted without parentheses; passing args to
+     * them would produce DDL that ClickHouse rejects at parse time.
+     */
+    private static function algorithmAcceptsArgs(SkipIndexAlgorithm $algorithm): bool
+    {
+        return match ($algorithm) {
+            SkipIndexAlgorithm::MinMax,
+            SkipIndexAlgorithm::Inverted => false,
+            default => true,
+        };
     }
 }
diff --git a/src/Query/Schema/Table.php b/src/Query/Schema/Table.php
@@ -580,7 +580,15 @@ public function dataSkippingIndex(
         string $name = '',
     ): static {
         if ($name === '') {
-            $name = 'skip_' . \implode('_', $columns);
+            // Sanitise column names — substring matches like `event-type` or
+            // `ns.col` are valid SQL identifiers when quoted, but the
+            // generated index name must still pass the strict identifier
+            // regex on `SkipIndex`.
+            $sanitised = \array_map(
+                fn (string $c): string => \preg_replace('/[^A-Za-z0-9_]+/', '_', $c) ?? $c,
+                $columns,
+            );
+            $name = 'skip_' . \implode('_', $sanitised);
         }
 
         $this->skipIndexes[] = new SkipIndex($name, $columns, $algorithm, $algorithmArgs, $granularity);
diff --git a/tests/Query/Schema/ClickHouseTest.php b/tests/Query/Schema/ClickHouseTest.php
@@ -861,4 +861,103 @@ public function testTableSettingsRejectsInvalidValue(): void
             $table->settings(['ok_key' => "evil'; DROP TABLE x; --"]);
         });
     }
+
+    public function testDataSkippingIndexNoArgAlgorithmRejectsArgs(): void
+    {
+        $this->expectException(ValidationException::class);
+        $this->expectExceptionMessage('minmax does not accept algorithm arguments.');
+
+        $schema = new Schema();
+        $schema->create('events', function (Table $table) {
+            $table->bigInteger('id')->primary();
+            $table->integer('score');
+            $table->dataSkippingIndex(['score'], SkipIndexAlgorithm::MinMax, algorithmArgs: [3]);
+        });
+    }
+
+    public function testDataSkippingIndexInvertedRejectsArgs(): void
+    {
+        $this->expectException(ValidationException::class);
+
+        $schema = new Schema();
+        $schema->create('events', function (Table $table) {
+            $table->bigInteger('id')->primary();
+            $table->string('text');
+            $table->dataSkippingIndex(['text'], SkipIndexAlgorithm::Inverted, algorithmArgs: [42]);
+        });
+    }
+
+    public function testDataSkippingIndexAutoNameSanitisesNonIdentifierColumns(): void
+    {
+        $schema = new Schema();
+        $result = $schema->create('events', function (Table $table) {
+            $table->bigInteger('id')->primary();
+            $table->string('event-type');
+            $table->dataSkippingIndex(['event-type'], SkipIndexAlgorithm::BloomFilter);
+        });
+        $this->assertBindingCount($result);
+
+        // Auto name: skip_event_type (non-identifier chars collapsed to _)
+        $this->assertSame(
+            'CREATE TABLE `events` (`id` Int64, `event-type` String,'
+            . ' INDEX `skip_event_type` `event-type` TYPE bloom_filter GRANULARITY 1)'
+            . ' ENGINE = MergeTree() ORDER BY (`id`)',
+            $result->query,
+        );
+    }
+
+    public function testDataSkippingIndexFloatArgAvoidsScientificNotation(): void
+    {
+        $schema = new Schema();
+        $result = $schema->create('events', function (Table $table) {
+            $table->bigInteger('id')->primary();
+            $table->string('user_id');
+            // 1e-5 false positive rate: the bug pre-fix is `(string) 1e-5` returning "1.0E-5"
+            $table->dataSkippingIndex(['user_id'], SkipIndexAlgorithm::BloomFilter, algorithmArgs: [1.0e-5]);
+        });
+        $this->assertBindingCount($result);
+
+        $this->assertStringContainsString('TYPE bloom_filter(0.00001)', $result->query);
+        // Numeric arg should be fixed-point — no 'E-' or 'E+' anywhere
+        $this->assertDoesNotMatchRegularExpression('/[Ee][+-]\d/', $result->query);
+    }
+
+    public function testAlterAddSkipIndex(): void
+    {
+        $schema = new Schema();
+        $result = $schema->alter('events', function (Table $table) {
+            $table->dataSkippingIndex(['user_id'], SkipIndexAlgorithm::BloomFilter);
+        });
+        $this->assertBindingCount($result);
+
+        $this->assertSame(
+            'ALTER TABLE `events` ADD INDEX `skip_user_id` `user_id` TYPE bloom_filter GRANULARITY 1',
+            $result->query,
+        );
+    }
+
+    public function testAlterAddSkipIndexComposite(): void
+    {
+        $schema = new Schema();
+        $result = $schema->alter('events', function (Table $table) {
+            $table->dataSkippingIndex(['user_id', 'event'], SkipIndexAlgorithm::Set, granularity: 4, algorithmArgs: [100], name: 'idx_user_event');
+        });
+        $this->assertBindingCount($result);
+
+        $this->assertSame(
+            'ALTER TABLE `events` ADD INDEX `idx_user_event` (`user_id`, `event`) TYPE set(100) GRANULARITY 4',
+            $result->query,
+        );
+    }
+
+    public function testAlterRejectsSettings(): void
+    {
+        $this->expectException(UnsupportedException::class);
+        $this->expectExceptionMessage('SETTINGS');
+
+        $schema = new Schema();
+        $schema->alter('events', function (Table $table) {
+            $table->settings(['index_granularity' => 4096]);
+        });
+    }
 }
