feat: add ALTER TABLE ... OPTIMIZE INDEX for incremental index maintenance - #755
feat: add ALTER TABLE ... OPTIMIZE INDEX for incremental index maintenance#755puchengy wants to merge 4 commits into
Conversation
…nance
Incremental scalar-index maintenance (index only the unindexed/newly-appended
fragments) was previously reachable only through the SDK `Dataset.optimizeIndices`.
The SQL surface offered no equivalent: `CREATE INDEX` (same name) is a full
distributed rebuild over all fragments, and `OPTIMIZE` compacts fragments only and
does not train indexes.
This adds a new SQL statement:
ALTER TABLE t OPTIMIZE INDEX [idx] [WITH (retrain = <bool>, num_indices_to_merge = <n>)]
It merges the unindexed fragments into existing indexes via lance-core's
`optimizeIndices`. Omitting the index name optimizes all indexes. Runs on the
driver (single node), consistent with the SDK path.
Changes:
- Grammar: new `#optimizeIndex` alternative in LanceSqlExtensions.g4 (reuses the
existing OPTIMIZE/INDEX/WITH tokens).
- Logical plan `OptimizeIndex` + physical `OptimizeIndexExec` (mirrors DropIndexExec;
maps WITH args to OptimizeOptions), wired in LanceDataSourceV2Strategy.
- `visitOptimizeIndex` added to the AST builder for all Spark version modules
(3.4 / 3.5 / 4.x).
- Docs: create-index.md now documents OPTIMIZE INDEX as the incremental path.
- Tests: AST unit tests (index name, all-indexes, WITH args) and an end-to-end
integration test populating a deferred zonemap via SQL.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The SQL surface and driver-side delegation are a sound way to expose incremental index maintenance, but the new command can acknowledge work it did not perform and silently reinterpret its options. A viable revision should validate the named-index and WITH-option contract at the Spark boundary before calling lance-core, then report success only for a matched operation. This includes constraining and documenting retrain to the index types core supports.
| val argsMap = args.map(t => (t.name, t)).toMap | ||
|
|
||
| indexName.foreach(name => builder.indexNames(List(name).asJava)) | ||
| argsMap.get("retrain").foreach(t => builder.retrain(t.value.asInstanceOf[Boolean])) | ||
| argsMap.get("num_indices_to_merge").foreach(t => | ||
| builder.numIndicesToMerge(t.value.asInstanceOf[Long].toInt)) |
There was a problem hiding this comment.
The SQL option boundary silently changes valid requests: unquoted identifiers retain their spelling, but this map is queried only with lowercase literals, so WITH (RETRAIN = TRUE) is accepted and executes with retrain=false. Unknown keys are ignored, duplicates become last-write-wins, and Long.toInt wraps out-of-range values (4294967296 becomes 0), changing optimization behavior. Normalize unquoted option names and validate the allowed set, uniqueness, value types, and integer range before building the core options; also constrain and document retrain to the index families supported by core.
Reproducer
I ran a reflection test against this head that constructed:
OptimizeIndexExec(
null, null, None,
Seq(LanceNamedArgument("RETRAIN", true)))and invoked buildOptions; asserting options.isRetrain failed because the observed value was false.
There was a problem hiding this comment.
The general option validation is improved, but this contract root remains in two places.
- lance-core 11.0.0-beta.3 documents
retrainas supported only for v3 vector indices. I ran a fully covered zonemap regression that recorded the dataset version, executedOPTIMIZE INDEX idx_retrain WITH (retrain = true), and asserted that the version advanced; SQL returnedoptimized, but the assertion failed withexpected: <true> but was: <false>. The added deferred-zonemap test cannot distinguish retraining from ordinary incremental population. Reject or removeretrainfor unsupported targets (including mixed all-index requests), or support it in core and add evidence from a real vector rebuild. arg.name.toLowerCaseuses the process locale. A reflection regression set the default locale totr-TR, passedRETRAIN, invokedbuildOptions, and failed withUnsupported OPTIMIZE INDEX option(s): retraın. UseLocale.ROOTfor normalization.
There was a problem hiding this comment.
Option validation, retrain rejection, and option-name Locale.ROOT normalization are fixed. The same locale-sensitive normalization root remains in the newly added strategy branch: indexName.map(_.toLowerCase) uses the process locale. With default locale tr-TR, an existing idx_i targeted as IDX_I becomes ıdx_ı, and exact validation rejects it. Normalize this path with Locale.ROOT and keep a locale regression; aligning create/drop index normalization would keep the SQL index-name contract consistent.
Reproducer
I added a focused Spark 3.5 test that creates idx_i, switches the default locale to tr-TR, and executes ALTER TABLE ... OPTIMIZE INDEX IDX_I, then ran:
task_tmp_dir=$(mktemp -d /home/agent/tmp/gate755-index-locale.XXXXXX)
JAVA_TOOL_OPTIONS="-Djava.io.tmpdir=$task_tmp_dir" ./mvnw test -pl lance-spark-3.5_2.13 -Dtest="AddIndexTest#testOptimizeIndexNameCaseInsensitiveLocaleIndependent"The test errored with Index ıdx_ı does not exist ... Existing indexes: idx_i from OptimizeIndexExec.
There was a problem hiding this comment.
Fixed on 2702be8d: CREATE, DROP, and OPTIMIZE index names now share LanceSystemIndex.normalizeName, which uses Locale.ROOT. I reran the previously failing end-to-end Spark 3.5 scenario—create idx_i, switch to tr-TR, then optimize IDX_I—and it passed. The committed helper tests also pass on both Scala 2.12 and 2.13. Closing this finding.
…to core Addresses lance-gatekeeper review on lance-format#755. - Missing named index no longer reports success: lance-core exact-matches indexNames and treats an empty match set as a successful no-op, so a typo was reported as completed maintenance. OptimizeIndexExec now checks the index exists via listIndexes() before calling optimizeIndices, and errors otherwise. - WITH-option boundary is now validated instead of silently reinterpreted: option names are normalized case-insensitively (so an unquoted, upper-cased RETRAIN is honored rather than running with retrain=false), unknown names and duplicates are rejected, value types are checked, and num_indices_to_merge is range-checked against int (no more Long.toInt wraparound, e.g. 4294967296 -> 0). - Documented retrain accurately (a source rebuild of the targeted index, still cheaper than drop+recreate; supported across scalar and vector index families in core) and the num_indices_to_merge contract. - Tests: OPTIMIZE INDEX on a missing name fails; an unknown option fails; an upper-cased RETRAIN option is applied and covers all fragments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The patch fixes ordinary missing-index handling and most WITH validation, but both prior contract roots remain: hidden system-index targets still report false success, and option handling still promises unsupported scalar retraining and depends on the JVM locale.
A viable revision should validate only user-optimizable index names, align retrain with the v3-vector-only contract in lance-core (including all-index requests), and normalize SQL option names with Locale.ROOT.
…e.ROOT Addresses lance-gatekeeper's second review on lance-format#755. - System indexes no longer report false success: lance-core filters system indexes (__lance_frag_reuse, __lance_mem_wal) before optimizing, so naming one was reported as completed maintenance. Validation now runs against the user-optimizable set (the same system-index filtering SHOW INDEXES uses) and rejects system-index names explicitly. Extracted the shared list into LanceSystemIndex so SHOW INDEXES and OPTIMIZE INDEX cannot drift. - Dropped the `retrain` option from the SQL surface: in lance-core 11.0.0-beta.3 retrain is a v3-vector-index rebuild and is a no-op for the scalar indexes this incremental command targets, so exposing it promised behavior it could not deliver. It remains available via Dataset.optimizeIndices in the SDK. `retrain` is now rejected as an unsupported option. - Option-name normalization uses Locale.ROOT instead of the process locale, so RETRAIN/NUM_INDICES_TO_MERGE parse correctly under locales like tr-TR. - Docs and tests updated: system-index rejection, retrain rejection, and a locale-independent case-insensitive option test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The missing/system-index and unsupported-option contracts are now fixed. One acceptance issue remains: case-insensitive index targeting still depends on the JVM default locale, so valid names can be rejected on affected installations.
Normalize index names with Locale.ROOT in the strategy path and retain the Turkish-locale regression; aligning create and drop index normalization will keep the SQL contract consistent.
Addresses lance-gatekeeper's third review on lance-format#755. Index-name lower-casing in LanceDataSourceV2Strategy used the JVM default locale, so under locales like tr-TR 'I' maps to a dotless 'ı' and a valid index name could be created under one spelling and rejected by OPTIMIZE INDEX (or DROP INDEX) under another. All three index DDL paths (CREATE / DROP / OPTIMIZE INDEX) now normalize via a shared LanceSystemIndex.normalizeName using Locale.ROOT, so the case-insensitive SQL contract is locale-independent and consistent across commands. - Added LanceSystemIndex.normalizeName (Locale.ROOT) alongside the existing system-index filter; strategy delegates all index-name normalization to it. - Added LanceSystemIndexTest with a tr-TR normalization regression plus isSystemIndex/null coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The locale-dependent index-name path is now fixed, so all earlier acceptance findings are addressed. The SQL command preserves the intended incremental-maintenance contract: it validates user index names and options before delegating to lance-core, rejects unsupported retrain and system-index targets, and normalizes identifiers consistently across CREATE, DROP, and OPTIMIZE.
Summary
Implements #743 (sub-task of #742). Adds a SQL surface for incremental scalar-index maintenance. Today, indexing only the newly-appended (unindexed) fragments is reachable exclusively through the SDK
Dataset.optimizeIndices. The SQL layer has no equivalent:ALTER TABLE ... CREATE INDEX <same name>is a full distributed rebuild over all fragments (atomically replaces the index).OPTIMIZE <table>compacts fragments only and does not train indexes.This PR adds:
It delegates to lance-core's
Dataset.optimizeIndices, so it merges only unindexed fragments (existing coverage is not recomputed). Omitting the index name optimizes all user indexes. It runs on the driver (single node), consistent with the SDK path.This keeps a clean separation of verbs:
CREATE INDEX= define/rebuild,OPTIMIZE INDEX= incremental maintenance.Options and validation
Option names are case-insensitive (normalized with
Locale.ROOT, so they are stable under locales such astr-TR). SupportedWITHoption:num_indices_to_merge0appends the new data as a fresh delta segment instead of merging into existing ones.The command validates its inputs at the Spark boundary rather than reporting work it did not do:
__lance_frag_reuse,__lance_mem_wal) that lance-core filters out before optimizing, is rejected instead of silently reported asoptimized. Validation uses the same system-index filtering asSHOW INDEXES.num_indices_to_merge(int range) are rejected.retrainis not accepted here (see follow-ups).Changes
#optimizeIndexalternative inLanceSqlExtensions.g4(reuses the existingOPTIMIZE/INDEX/WITHtokens; no new keywords).OptimizeIndexlogical plan +OptimizeIndexExecphysical exec (modeled onDropIndexExec; validatesWITHoptions and mapsnum_indices_to_mergetoOptimizeOptions), wired intoLanceDataSourceV2Strategy.visitOptimizeIndexadded across all Spark version modules (3.4 / 3.5 / 4.x).LanceSystemIndexcentralizes the system-index name set +isSystemIndex, and locale-independent index-name normalization (normalizeName,Locale.ROOT).SHOW INDEXESnow uses it too, and all three index DDLs (CREATE / DROP / OPTIMIZE INDEX) normalize names through it so the case-insensitive contract is consistent and locale-independent.create-index.mddocumentsOPTIMIZE INDEXas the incremental path.Testing
LanceSqlExtensionsAstBuilderTest(3.4 + 3.5): index name, all-indexes (name omitted), andWITH (...)args parse cases.LanceSystemIndexTest: locale-independent normalization (incl. atr-TRregression),isSystemIndex, and null handling.AddIndexTest(integration, SQL-driven): deferred zonemap populated viaOPTIMIZE INDEX; missing-name rejected; system-index (__lance_frag_reuse) rejected; unknown option rejected;retrainrejected; case-insensitive option name honored.ShowIndexesTest: still green after the shared-helper refactor.compileon base / 3.4 / 3.5 / 4.2);spotless:checkclean.AddIndexTestbtree cases fail locally with a DataFusion external-sort OOM; these are environmental (constrained sandbox) and reproduce onmainwithout this change.Notes / follow-ups
optimizeIndices); making it distributed is a natural follow-up but out of scope here.retrain(rebuild an index's model from source data) is intentionally not exposed through this SQL command: in lance-core it applies only to v3 vector indexes and is a no-op for the scalar indexes this command targets, so exposing it via SQL would promise behavior it cannot deliver. It remains available viaDataset.optimizeIndicesin the SDK. Exposing it for vector targets from SQL is a possible follow-up, ideally paired with lance-core returning an explicit error for unsupported targets (rather than a silent no-op) so the Spark layer does not have to re-encode the v3-vector contract. (retrainpass-through is listed in the Support OPTIMIZE INDEX SQL #743 scope.)OPTIMIZE INDEXis tracked separately in Support distributed execution for OPTIMIZE INDEX #750; automatic size-tiered merge selection in Support size-tiered scalar index optimization #744.🤖 Generated with Claude Code