Doris-Breakwater commented on issue #66772: URL: https://github.com/apache/doris/issues/66772#issuecomment-5290749192
Breakwater-GitHub-Analysis-Slot: slot_13a9e214821c ### Initial triage **Judgment: valid, high-confidence `branch-4.1` Lance REST integration bugs.** There are two independent FE-side compatibility defects: credential alias normalization drops Gravitino-vended options on the BE path, and Doris assumes that the configured parent namespace is itself table-listable. Problem 1 blocks scans that rely on credential vending; Problem 2 creates an unusable advertised database and rejects an otherwise reachable catalog when `test_connection=true`. The issue currently has no labels, assignee, milestone, or earlier comments, and the reporter is willing to submit a PR. I checked the exact reported Doris commit (`0e53b31f58674a4716f12cdab4c46f3cb607ed05`) and the Apache Gravitino 1.3.0 tag (`40fdf6ab96ac87b47e6d3e14e7c4dc0d815e68f0`). I did not rerun the full MinIO/Gravitino fixture locally; the runtime A/B evidence below is reporter-supplied, while the failure mechanisms are source-confirmed. ### Verified facts #### 1. Vended credentials are lost only on the FE-to-BE conversion path - Doris explicitly requests credentials in [`describeTable`](https://github.com/apache/doris/blob/0e53b31f58674a4716f12cdab4c46f3cb607ed05/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceExternalCatalog.java#L320-L330). It merges the returned `storage_options` unchanged into the Java SDK options, but separately calls `LanceStorageOptions.forBackend(...)` for the scan path ([lines 288-313](https://github.com/apache/doris/blob/0e53b31f58674a4716f12cdab4c46f3cb607ed05/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceExternalCatalog.java#L288-L313)). This explains why FE metadata loading can succeed while the BE scan fails. - [`forBackend`](https://github.com/apache/doris/blob/0e53b31f58674a4716f12cdab4c46f3cb607ed05/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceStorageOptions.java#L55-L69) recognizes only `aws_access_key_id`, `aws_secret_access_key`, `aws_session_token`, `aws_endpoint`, `aws_region`, and `aws_virtual_hosted_style_request`. With no static credentials, Gravitino's unprefixed `access_key_id`, `secret_access_key`, `endpoint`, and `region` therefore contribute no BE properties. - The BE does not receive the raw vended map. [`LanceScanNode.getLocationProperties()`](https://github.com/apache/doris/blob/0e53b31f58674a4716f12cdab4c46f3cb607ed05/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/source/LanceScanNode.java#L250-L253) returns the converted map, and [`LanceTableReader::_storage_options`](https://github.com/apache/doris/blob/0e53b31f58674a4716f12cdab4c46f3cb607ed05/be/src/format_v2/table/lance_reader.cpp#L897-L928) reads only Doris's normalized `AWS_*`/`use_path_style` keys. There is no later opportunity for the unprefixed values to recover. - Gravitino intentionally strips `lance.storage.` from catalog/table properties in [`LancePropertiesUtils`](https://github.com/apache/gravitino/blob/40fdf6ab96ac87b47e6d3e14e7c4dc0d815e68f0/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/utils/LancePropertiesUtils.java#L35-L69), and its table-description path returns that resolved map as `storage_options` ([source](https://github.com/apache/gravitino/blob/40fdf6ab96ac87b47e6d3e14e7c4dc0d815e68f0/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java#L137-L150)). Its tests explicitly expect `access_key_id`, `endpoint`, and `region` without `aws_` prefixes. - The reporter's positive control—same Doris catalog and physical dataset, changing only the vended key spelling to `aws_*`, after which all 1024 rows scan—matches this exact branch in the code. This is not explained by the Java namespace library version difference. #### 2. The root database and connectivity probe use an invalid Gravitino namespace depth - [`listDatabaseNames`](https://github.com/apache/doris/blob/0e53b31f58674a4716f12cdab4c46f3cb607ed05/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceExternalCatalog.java#L158-L191) unconditionally adds `rootDatabase`. The root name maps back to an empty relative namespace, so [`listTableNamesFromRemote`](https://github.com/apache/doris/blob/0e53b31f58674a4716f12cdab4c46f3cb607ed05/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceExternalCatalog.java#L221-L245) calls `ListTables` on the configured parent itself. - For `lance.namespace.parent=lance_catalog`, that request has one level. Gravitino's native backend accepts a one-level identifier for `ListNamespaces` and returns its schemas ([source](https://github.com/apache/gravitino/blob/40fdf6ab96ac87b47e6d3e14e7c4dc0d815e68f0/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceNameSpaceOperations.java#L88-L126)), but requires **exactly two levels** for `ListTables` ([source](https://github.com/apache/gravitino/blob/40fdf6ab96ac87b47e6d3e14e7c4dc0d815e68f0/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceNameSpaceOperations.java#L435-L454)). The reported error is therefore deterministic for this mapping. - [`checkWhenCreating`](https://github.com/apache/doris/blob/0e53b31f58674a4716f12cdab4c46f3cb607ed05/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceExternalCatalog.java#L113-L139) calls `ListTables(parent)` before `ListNamespaces(parent)`, so `test_connection=true` fails even though the namespace endpoint and the schema-level table endpoints are reachable. #### 3. Existing tests model only the successful spellings and a permissive root - The regression stub vends only `aws_*` keys ([source](https://github.com/apache/doris/blob/0e53b31f58674a4716f12cdab4c46f3cb607ed05/docker/thirdparties/docker-compose/iceberg/scripts/lance_rest_server.py#L137-L156)), while the scan regression deliberately has no static access/secret key. Thus the test exercises vending but not Gravitino's spelling. - The same stub allows `ListTables` at any namespace depth and places its default table directly at the empty/root namespace ([source](https://github.com/apache/doris/blob/0e53b31f58674a4716f12cdab4c46f3cb607ed05/docker/thirdparties/docker-compose/iceberg/scripts/lance_rest_server.py#L45-L65), [list implementation](https://github.com/apache/doris/blob/0e53b31f58674a4716f12cdab4c46f3cb607ed05/docker/thirdparties/docker-compose/iceberg/scripts/lance_rest_server.py#L96-L120)). It therefore cannot reveal the fixed-depth Gravitino behavior. - There is no focused FE unit test for `LanceStorageOptions.forBackend`; current coverage tests only the static Doris-to-Java direction. ### Missing information / remaining decision No additional version, log, profile, or reproduction information is required to accept and prioritize these two defects. The supplied versions, REST response, SQL reproduction, BE error, workaround, and spelling-only positive control are sufficient. A maintainer-side end-to-end run is still useful validation, but it is not a reporter blocker. One implementation contract needs an explicit maintainer decision: generic Lance namespaces and the existing filesystem/stub behavior can have tables directly under the configured parent, while Gravitino's catalog/schema model cannot. Removing the root database unconditionally would regress the former; continuing to add it unconditionally leaves a phantom database for the latter. The fix should preserve both models rather than infer support solely from namespace depth. ### Recommended next steps 1. **Normalize vended aliases centrally.** Extend `forBackend` (or a shared canonicalizer) to accept the unprefixed keys actually emitted by Gravitino as well as the existing `aws_*` forms, preserve the current precedence of vended values over static catalog defaults, and define deterministic precedence if both aliases are present. Cover endpoint/HTTP and addressing-style behavior, not only access/secret keys. Do not log credential values. 2. **Do not require the configured parent to be table-listable for connectivity.** Probe `ListNamespaces(parent)` first; if a child namespace exists, `ListTables` can be tested against that table-capable child. An empty catalog should still pass a namespace connectivity check. Do not catch and suppress arbitrary table-list failures, since that would hide authentication and service errors. 3. **Make root exposure capability-aware or explicitly configurable.** Include the root database only when tables directly under the configured parent are supported, or add a documented way to suppress it for catalog/schema backends. Keep the existing root behavior for filesystem/permissive namespaces. Whichever approach is chosen, every database returned by `SHOW DATABASES` must be listable. 4. **Add interoperability coverage.** Add a unit test for `forBackend` using Gravitino's unprefixed map and alias precedence; add a regression-stub mode that vends unprefixed keys with no static credentials; and add a fixed-depth mode where `ListNamespaces([catalog])` succeeds but `ListTables([catalog])` fails while `ListTables([catalog,schema])` succeeds. Retain a root-table case to prevent regressions in the generic model. 5. Add a REST URI base-path case (for example `/lance`) to tests/docs, since the current URI validation already permits it but the root-mounted stub does not exercise it. Suggested triage: add the repository's bug and Lance/external-catalog ownership labels, and let the willing reporter prepare the PR once the root-namespace contract is agreed. The code is currently `branch-4.1`-specific, so the target branch should follow the maintainers' branch policy. -- 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]
