Add create_index (fragment-parallel) / optimize_indices / compact table procedures - #188
Open
fl0-m wants to merge 3 commits into
Open
Add create_index (fragment-parallel) / optimize_indices / compact table procedures#188fl0-m wants to merge 3 commits into
fl0-m wants to merge 3 commits into
Conversation
Adds ALTER TABLE ... EXECUTE support for building and maintaining Lance scalar/FTS indexes directly from Trino, via org.lance.Dataset APIs the connector already depends on (lance-core:7.0.0): - create_index(column, index_type, ...) - builds an inverted (FTS) index - optimize_indices(...) - incrementally indexes newly appended fragments - compact(defer_index_remap => true) - compaction that doesn't force an index rebuild, via Lance's Fragment Reuse Index All three are coordinator-only (TableProcedureExecutionMode.coordinatorOnly()): none of them read or write table data through Trino's split/page pipeline, they call directly into the Lance dataset the same way lance-spark's own CREATE INDEX does. See lance-format#187 for the full design rationale, including why this is scoped to coordinator-only for now and what would be needed to delegate index builds to workers for very large tables. Closes lance-format#187
Phase 2 follow-up to the previous commit. When a fresh (train=true) index
build has more than one fragment, split the fragments into batches and
build one index segment per batch in parallel on the coordinator, then
merge and commit as a single logical index:
dataset.createIndex(fragmentIds=batch) // one per batch, read-locked,
// does not commit
dataset.mergeExistingIndexSegments(segments)
dataset.commitExistingIndexSegments(name, column, [merged])
This does not distribute across Trino worker nodes. Trino's distributed
ALTER TABLE EXECUTE machinery (TableProcedureExecutionMode.distributedWith
FilteringAndRepartitioning) fixes the scanned columns to the table's real
columns at analysis time (see BeginTableWrite#findTableScanHandleForTable
Execute in trino-main) - it's built for procedures like Iceberg's OPTIMIZE
that genuinely read and rewrite full rows. Lance's createIndex already
reads fragment data natively and doesn't need Trino to also move that data
through the page pipeline, so using that machinery here would mean paying
for a full-table read just to throw the pages away. Parallelizing across
the coordinator's cores gets the CPU-bound tokenize/build speedup without
that redundant I/O.
Two non-obvious things learned from the native errors while wiring this up
(both now called out in code comments):
- Despite IndexOptions.withIndexUUID's Javadoc ("multiple fragment-level
indices need to share UUID for later merging"), passing one shared UUID
across parallel createIndex calls fails at mergeExistingIndexSegments
with "duplicate segment uuid" - each batch needs its own auto-generated
UUID (leave withIndexUUID unset).
- Each per-fragment createIndex call validates the target index name
against the dataset's currently committed indices immediately, even
though it doesn't commit anything - so replace => true has to drop the
existing index before building segments, not just before the final
commitExistingIndexSegments call.
TestLanceTableExecuteProcedures#testCreateIndexParallelizesAcrossMultiple
Fragments exercises this against 4 separately-inserted fragments and
verifies row-level correctness through the merge. Full regression suite
(TestLanceMetadata, TestLancePlugin, TestLanceTableHandle,
TestLanceDirectorySingleLevelConnectorTest incl. its ~370 inherited
BaseConnectorTest cases) still passes.
The previous commit shared one Dataset object across all parallel createIndex(fragmentIds=...) calls. Benchmarking (real data, 1M rows, forced parallelism 1/4/8) showed this was consistently slower than the plain non-split path, and got worse with more threads: parallelism=1 shared=1.15s isolated=0.83s parallelism=4 shared=1.11s isolated=0.64s parallelism=8 shared=1.04s isolated=0.66s This was NOT because Lance's createIndex is internally parallelized - lance-index's inverted-index builder (rust/lance-index/src/scalar/ inverted/builder.rs, ~4300 lines) has no rayon/tokio/thread concurrency at all. The actual cause was contention from sharing one Dataset's allocator/native handle across threads. buildIndexSegmentsInParallel now takes (tablePath, storageOptions, ...) instead of a shared Dataset, and each batch opens its own independent Dataset.open(tablePath, ...) handle, matching what N separate worker processes would naturally do, with no shared in-process state. Full regression suite (267 inherited BaseConnectorTest cases plus all procedure tests) still passes.
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements #187:
ALTER TABLE ... EXECUTEtable procedures that build and maintain Lance indexes directly from Trino, without needing the Python/Rust SDK out-of-band. This now includes both phases sketched in the issue, though phase 2 landed in a different (and, after a correction below, differently-implemented) shape than originally planned — see below.All three procedures are
TableProcedureExecutionMode.coordinatorOnly()— they call directly intoorg.lance.Dataset#createIndex/#optimizeIndices/#compact(already on the classpath via the existinglance-core:7.0.0dependency), the same APIs lance-spark's ownCREATE INDEXuses. No new Lance dependency.create_indexcurrently only supportsindex_type => 'fts'; other Lance scalar index types (btree/zonemap/bitmap/ngram) fit the same shape and are a natural, small follow-up rather than part of this PR.Changes
LanceTableProcedures— registers the three procedures + theirALTER TABLE ... EXECUTEargument schemas viaConnector#getTableProcedures().LanceMetadata#getTableHandleForExecute/#executeTableExecute— parses arguments into per-procedure handles and dispatches toDataset.LanceTableExecuteHandle/LanceProcedureHandle/Lance{CreateIndex,OptimizeIndices,Compact}Handle— plain Java records implementing the SPI handle interfaces, with@JsonTypeInfo/@JsonSubTypeson the polymorphicprocedureHandlefield (mirrors Iceberg'sIcebergTableExecuteHandle/IcebergProcedureHandle).create_indexfragment-parallel build (see "Phase 2" below): when a fresh (train => true) build spans more than one fragment,LanceMetadata#executeCreateIndexsplits the fragments into batches sized toRuntime.getRuntime().availableProcessors()and builds one index segment per batch concurrently, each on its own independently-openedDatasethandle (buildIndexSegmentsInParalleltakestablePath/storageOptions, not a sharedDataset— see the correction below for why), then consolidates viamergeExistingIndexSegmentsand commits once viacommitExistingIndexSegments.TestLanceTableExecuteProcedures— end-to-end coverage: builds an FTS index, verifies duplicate-without-replacefails andreplace => truesucceeds (both single-fragment and fragment-parallel paths), validates required/invalid arguments, exercises insert →optimize_indices→ insert →compact(defer_index_remap => true), exercises the fragment-parallel build+merge across 4 separately-inserted fragments with row-level correctness checks, and verifiesbase_tokenizer => 'ngram'works over mixed Chinese/Japanese/English text (script-agnostic tokenization, doesn't rely on whitespace word boundaries).Two non-obvious gotchas worth flagging for reviewers
1. Coordinator-only procedures still need JSON-serializable handles. Even a fully
coordinatorOnly()procedure needs itsConnectorTableExecuteHandleto be Jackson-serializable: Trino still builds aSimpleTableExecuteNodeinto the query plan and serializes it as part of the (coordinator-local)TaskUpdateRequest. My first pass used plain classes with fluent no-get-prefix accessors and it failed at runtime withNo serializer found for class LanceTableExecuteHandle ... no properties discovered— Jackson's default bean introspection doesn't pick those up. Switching to Java records (same pattern Iceberg uses forIcebergTableExecuteHandle) fixed it. This only surfaces at runtime against a liveDistributedQueryRunner, not in aTestLanceMetadata-style unit test that callsgetTableHandleForExecute/executeTableExecutedirectly without ever serializing anything — so it would have shipped broken without the integration-style test.2.
IndexOptions.withIndexUUID's Javadoc is misleading for this use case. It says: "A UUID to use for fragment-level distributed indexing — multiple fragment-level indices need to share UUID for later merging." Passing one shared UUID across parallelcreateIndex(fragmentIds=...)calls actually fails atmergeExistingIndexSegmentswithduplicate segment uuid ... for index. LeavingwithIndexUUIDunset (so each call gets its own auto-generated UUID) is what works. Separately, each per-fragmentcreateIndexcall validates the target index name against the dataset's currently committed indices immediately — even though it doesn't commit anything itself — soreplace => truehas to drop the existing index before building segments, not just before the finalcommitExistingIndexSegmentscall, or every batch fails with "Index name already exists." Both are called out in code comments at the call sites.Correction: the original "Phase 2" reasoning was partly wrong — fixed in a follow-up commit
The first version of this PR parallelized fragment builds by sharing one
Datasetobject across threads, on the (unverified) assumption that concurrentcreateIndex(fragmentIds=...)calls against a shared handle would scale fine since each only takes a read lock. A reviewer asked me to actually verify this instead of taking it on faith, and benchmarking (real data, 1M rows, forced parallelism 1/4/8) showed the shared-Datasetversion was slower than not parallelizing at all, and got worse with more threads:Dataset(original)Datasetper thread (now)This was not because Lance's
createIndexis internally parallelized — I checkedrust/lance-index/src/scalar/inverted/builder.rs(the actual FTS build code, ~4,300 lines) and there's norayon/tokio::spawn/thread::spawnanywhere in it. The real cause was contention from sharing oneDataset's allocator/native handle across threads. The fix (now in this PR): each parallel batch opens its own independentDataset.open(tablePath, ...)handle instead of sharing one — matching what genuinely separate processes would naturally do, with no shared in-process state. Full regression suite still passes with this change.What about genuine worker-node distribution (not just coordinator threads)?
Also investigated, since the isolated-
Datasetresult raised the natural follow-up: if independent handles scale better, why not actual separate Trino worker processes? Two things came out of that:ALTER TABLE ... EXECUTE's existing distributed mode with a placeholder-scan trick (the connector's own page source/sink control how expensive "reading" the required columns actually is, so it can do the realcreateIndexwork as a worker-side side effect and only nominally satisfy the engine's full-column-scan plan-shape requirement), or (b) aConnectorTableFunction-based fallback, precedented by Trino's ownSequenceFunction.lance-core, not in Trino or this connector:org.lance.index.Index(the objectcreateIndex(fragmentIds=...)returns, needed for the coordinator-sidemergeExistingIndexSegments/commitExistingIndexSegmentsstep) doesn't implementSerializableand has a private constructor, so it can't cross a JVM process boundary at all. This turns out to be a known, currently-unresolved limitation — see Unified API or Separate APIs for Distributed Index Building? lance#5359, an open RFC from the Lance maintainers describing exactly this ("merge_metadata... is invoked at the Python/Java layer by directly calling into Index, and there isn't a unified entry point for this at the Rust level").Full write-up (both Trino-side designs, the blocker, and a concrete large-scale motivating scenario) is in #189. Not blocking this PR — coordinator-local parallelism (now fixed above) is what's shipping here.
optimize_indicesandcompactare unchanged from the coordinator-only design — I don't think they're good parallelization candidates:optimizeIndicesis documented as single-node even in Lance's own Python/Ray ecosystem (Ray's distributed indexing uses a separate lower-levelcreate_fragment_indexAPI, notoptimizeIndices), so there's no distributed primitive to delegate to without reimplementing unindexed-fragment detection ourselves.I've updated #187 with the same corrections so the issue reflects what actually shipped rather than the original (partly incorrect) plan.
🤖 Generated with Claude Code