Gabriel39 commented on issue #66497: URL: https://github.com/apache/doris/issues/66497#issuecomment-5199781913
Thanks for the detailed proposal. The source-of-truth model, logical/physical index distinction, version semantics, and post-commit metadata refresh direction are sound. After comparing the proposal with the current Doris command path, the pinned Lance SDK/Namespace API, and the Lance Spark, StarRocks, ClickHouse, DuckDB, and Trino designs, I think the following points should be resolved before implementation. ## Blocking concerns ### 1. A synchronous native build in FE is not a safe production execution model The proposal already lists the essential problems: index creation performs data scans and native CPU/memory/object-store I/O in FE, while the pinned API has no reliable cancellation/progress mechanism and FE failover can leave the result ambiguous. This also bypasses Doris workload management and allows a user with ALTER to trigger unbounded native work in the control plane. The closest Lance integration distributes fragment builds to Spark executors and leaves the driver to coordinate/finalize the commit. ClickHouse similarly separates index metadata operations from the heavy MATERIALIZE INDEX mutation. - https://lance.org/integrations/spark/operations/ddl/create-index/ - https://clickhouse.com/docs/reference/statements/alter/skipping-index My recommendation is an asynchronous job with FE orchestration and BE or external-service execution, including status, cancellation, concurrency limits, and retry/reconciliation. If FE-side execution must exist temporarily, it should be explicitly experimental, disabled by default, and protected by dataset-size/fragment/concurrency limits. It should not be presented as the final production lifecycle implementation. ### 2. Reusing the current Doris index command chain requires a larger refactor than the proposal describes The current parser only accepts NGRAM_BF, INVERTED, and ANN; it does not accept IVF_PQ, BTREE, BITMAP, or OR REPLACE: https://github.com/apache/doris/blob/e3289c1a5df7558cb8e63d80379d4edebf9c498c/fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4#L237-L240 More importantly, CreateIndexOp.validate() always translates the definition into the persisted internal catalog.Index, while AlterTableCommand currently rejects CreateIndexOp for external tables: https://github.com/apache/doris/blob/e3289c1a5df7558cb8e63d80379d4edebf9c498c/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateIndexOp.java#L73-L89 That conflicts with the stated goal of keeping Lance index DTOs separate from Doris internal index serialization. The design should explicitly introduce a neutral parsed index definition, resolve the target table/catalog first, and then choose internal-index validation/serialization or Lance-specific validation/dispatch. Adding only a LanceIndexOperator after the existing validation path will not be sufficient. For vector syntax, I would prefer reusing the Doris category-level surface: ~~~sql CREATE INDEX idx ON lance_ctl.db.tbl (vec_col) USING ANN PROPERTIES ( "index_type" = "IVF_PQ", ... ); ~~~ This also follows the StarRocks pattern of USING VECTOR plus an index_type property and avoids adding a grammar keyword for every Lance algorithm. ## Other required revisions ### 3. Incremental maintenance is necessary for a usable external index lifecycle External writers can append fragments immediately after an index is created. Coverage will then decay. CREATE OR REPLACE INDEX performs an expensive full rebuild and is not an adequate maintenance operation. The pinned Java SDK exposes Dataset.optimizeIndices(), and Lance documents it as the incremental path for indexing newly appended fragments. I suggest adding a separate operation such as: ~~~sql BUILD INDEX idx ON lance_ctl.db.tbl; -- or ALTER TABLE lance_ctl.db.tbl OPTIMIZE INDEX idx; ~~~ The tests must also prove that queries include unindexed fragments correctly. Otherwise partial coverage becomes a correctness issue, not merely a performance issue. ### 4. Do not put unbounded physical segment metadata in SHOW INDEX.Properties A JSON segments array can become extremely large for a fragmented or distributed index. It can create FE memory, MySQL result packet, and client usability problems. Lance Spark intentionally returns only logical summaries from SHOW INDEXES and excludes per-segment metadata: https://lance.org/integrations/spark/operations/ddl/show-indexes/ I recommend keeping the existing 13-column SHOW INDEX surface for logical name, columns, type, and aggregate coverage only. UUIDs, dataset versions, and fragment sets should be exposed through a structured and filterable system table/TVF such as lance_index_segments, not embedded in one string field. ### 5. REST behavior should be capability-based, not rejected by catalog type The pinned lance-namespace-core 0.7.7 interface already defines createTableIndex, createTableScalarIndex, listTableIndices, describeTableIndexStats, and dropTableIndex: https://github.com/lance-format/lance-namespace/blob/v0.7.7/java/lance-namespace-core/src/main/java/org/lance/namespace/LanceNamespace.java#L394-L447 For a Directory Catalog, direct Dataset operations are appropriate. For a REST Catalog, index operations should go through the Namespace service when the server supports them; Doris should return a clear unsupported error when it does not. Directly mutating the dataset with vended credentials would bypass service-side authorization, managed versioning, and transaction policy. At minimum, REST SHOW INDEX should use list/stats operations in Phase 1. ### 6. Narrow and rename the initial type set I suggest Phase 1 support: - vector: IVF_PQ; - scalar: BTREE and BITMAP; - IVF_FLAT only after the Doris query path is verified end to end; - no INVERTED until the Lance FTS/BM25 query path is implemented. Lance INVERTED is an FTS index with different tokenizer/query semantics from a Doris internal inverted index. Exposing the same keyword before Doris can query it is misleading. A future SQL surface should call it FTS. The complete type/property matrix should be part of the design, not deferred entirely to the PR. It should include column types, vector element type/dimension/nullability, metric compatibility, parameter ranges, and build/query ABI compatibility. ### 7. Define SQL identifier semantics for index names Allowing both idx and IDX while matching names exactly makes DROP INDEX idx ambiguous under Doris/MySQL identifier behavior. We should preserve display case but define quoted/unquoted resolution and reject new case-only duplicates, or provide an unambiguous quoted-name rule for pre-existing external collisions. ### 8. Refresh/recovery must handle post-commit failures “Failed mutations do not refresh metadata” is insufficient when the Lance commit succeeds but FE fails before the edit log or client response. A later IF NOT EXISTS retry may be a no-op and still leave Doris caches stale. The implementation should distinguish pre-commit failure from unknown/post-commit outcome, reopen the latest snapshot to verify the postcondition, and invalidate/refresh Doris metadata even for a reconciled no-op. SHOW INDEX helps the user observe state but does not repair the query path cached metadata. Also, local/file catalogs need an explicit shared-filesystem or single-node restriction; otherwise a master failover can move execution to an FE that cannot see the same path. ## Answers to the review questions - **Q1:** No for the production implementation. FE-side synchronous execution is acceptable only as a tightly gated experimental fallback. - **Q2:** CREATE OR REPLACE INDEX is reasonable if replacement is atomic, but incremental BUILD/OPTIMIZE INDEX is more important. For vector indexes, prefer USING ANN plus index_type. - **Q3:** Keep SHOW INDEX as a logical summary; expose physical segments through a system table/TVF. - **Q4:** Start with IVF_PQ + BTREE + BITMAP; add IVF_FLAT only after end-to-end verification, and exclude INVERTED until FTS querying is available. - **Q5:** Support REST inspection through Namespace list/stats. Support REST mutation only through Namespace index operations and server capability detection, never by bypassing the metadata service. The proposal demonstrates a good understanding of the Lance index model, but the execution placement and the split between Doris internal index objects and external Lance operations need to be redesigned before implementation starts. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
