Gabriel39 commented on issue #66497: URL: https://github.com/apache/doris/issues/66497#issuecomment-5205105189
Thanks for the detailed revision. V2 addresses most of the architectural concerns from the first review: synchronous FE construction is no longer the production path, the neutral command model is clearer, BUILD INDEX is separated from full replacement, logical and physical metadata are separated, and the recovery states are substantially better defined. I think the Directory Catalog direction is now viable, but I do not think the design is implementation-ready yet. The following issues still need to be resolved. ### 1. The REST design currently promises more than the pinned Namespace API can express Doris pins Lance Namespace 0.7.7. Its `CreateTableIndexRequest` supports the column, index type, name, distance type, and FTS-related fields, but it does not carry `num_partitions`, `num_sub_vectors`, `zone_size`, replace semantics, or an uncovered-fragment/incremental-build request: https://github.com/lancedb/lance-catalog/blob/v0.7.7/java/lance-namespace-apache-client/docs/CreateTableIndexRequest.md There is also no separate Namespace BUILD INDEX operation. In addition, `ListTableIndices` exposes only name, UUID, columns, and status, while index stats expose only type, distance, indexed/unindexed row counts, and physical index count. That is not sufficient to verify that an existing REST index has the complete normalized definition required by the proposed `IF NOT EXISTS` reconciliation. Please either: - make the required Namespace specification/API changes an explicit prerequisite; or - define a Phase-1 REST capability matrix and reject unsupported replace, build, and property combinations instead of promising uniform behavior. “Advertised capabilities” should also be made concrete. In the pinned Java interface, optional operations are primarily represented by typed `UnsupportedOperationException`; there is no general capability-advertisement API. ### 2. Reusing SHOW/CANCEL BUILD INDEX requires a real external-job integration design The current `CancelBuildIndexCommand` explicitly rejects external catalogs and eventually looks up an internal `OlapTable` and `SchemaChangeHandler.IndexChangeJob`: https://github.com/apache/doris/blob/master/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CancelBuildIndexCommand.java Similarly, `SHOW BUILD INDEX` reads the internal `/jobs/<dbId>/build_index` proc data and has internal-index-specific columns such as `PartitionName` and `AlterInvertedIndexes`: https://github.com/apache/doris/blob/master/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowBuildIndexCommand.java Extending the SQL vocabulary is reasonable, but the design should specify the common job registry/proc source, catalog qualification, filtering, privilege checks, output schema, and state mapping for internal and Lance jobs. Otherwise the SQL examples in this proposal cannot work through the existing implementation. ### 3. REST reconciliation needs job-specific provenance, not only a matching postcondition The remote `transaction_id` is optional. If the service commits the operation but FE fails before persisting the returned transaction ID, a later “same name and matching definition” observation does not prove that this Doris job performed the commit; an external writer may have produced the same postcondition. This is particularly important because Lance allows concurrent CreateIndex transactions. For two operations on the same logical name, the later operation can replace the earlier one rather than returning the retryable conflict assumed by this proposal: https://lance.org/format/table/transaction/ Please persist a client operation ID before dispatch and propagate it through Namespace transaction properties or another queryable server-side idempotency/provenance mechanism. For Directory builds, expected segment UUIDs can provide this proof. Without job-specific evidence, the REST job should remain `OUTCOME_UNKNOWN` instead of being marked successful merely because the logical definition matches. ### 4. DROP also needs a durable operation record The proposal creates durable jobs for create, replace, and build, but allows DROP to complete synchronously while still promising failover-safe post-commit reconciliation and refresh. If DROP commits externally and FE fails before refresh/edit-log completion, there is no durable state from which to resume. DROP should therefore use the same durable Lance job/operation record, even if it normally moves through the states very quickly. ### 5. Please make the distributed segment contract explicit The pinned Java SDK already exposes `mergeExistingIndexSegments` and `commitExistingIndexSegments`, so BE construction followed by one FE commit is technically plausible: https://github.com/lance-format/lance/blob/v9.1.0-beta.3/java/src/main/java/org/lance/Dataset.java However, the design should require: - one pinned and verified Lance core/index-metadata compatibility contract across lance-c and the Java SDK; - validation of field IDs, physical index type and parameters, starting dataset version, exact assigned fragment coverage, UUID uniqueness, and artifact location before FE accepts a worker result; - for incremental BUILD, committing the complete final logical segment set (existing compatible segments plus new segments), not only the newly built segments; - an explicit IVF model strategy. Independent per-segment models are valid for fan-out query, but physical merge requires compatible/shared model artifacts such as centroids and PQ codebooks. See: https://lance.org/guide/distributed_indexing/ Credentials should not be persisted in the job edit log. Workers should obtain or refresh catalog credentials when a task or retry is dispatched. ### 6. Metadata details I recommend reporting `IVF_PQ`, rather than `ANN`, in `SHOW INDEX.Index_type`. ANN is the SQL category, while `IVF_PQ` is the actual index type; reporting only ANN becomes ambiguous when additional ANN implementations are added. REST metadata should also be explicitly capability-dependent: the pinned stats model does not expose fragment counts or the complete creation properties, so those SHOW fields cannot always be populated uniformly. ### Integration-model recommendation For the remaining planning question, I prefer direct BE execution through an extended lance-c for Directory Catalog. An external Java/Rust build service would introduce a new deployment, authentication, versioning, availability, and operational dependency for a core Apache Doris feature. Keeping execution in BE is a better fit as long as the production path remains gated until the required lance-c build, cancellation, progress, segment-metadata, and compatibility APIs are available. Section 5 already says that the proposal chooses BE execution, while the final section presents this as unresolved. Please make the conclusion consistent. Once the REST capability boundary, existing job-command integration, job-specific reconciliation, and durable DROP semantics are clarified, I think the design will be in much better shape for approval and decomposition into implementation issues. -- 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]
