zhangstar333 commented on PR #68349:
URL: https://github.com/apache/doris/pull/68349#issuecomment-6138566786
hi, @wangzhaobo957-cloud have some review major point by AI Reviewer, maybe
u could have a look.
### F-04 · Do not discard fragments when validating managed branch locations
- **Severity**: Major
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceManifestPaths.java:151-153`
- **Category**: correctness / caches / parallel paths
```java
String stripped =
StringUtils.substringBefore(StringUtils.substringBefore(location, "?"), "#");
if (!URL_SCHEME.matcher(stripped).find()) {
return normalizeObjectPath(StringUtils.strip(stripped, "/"));
```
**What is wrong.** The new location equivalence admits a URI fragment that
makes a branch read open the main dataset.
**Why it happens.**
`fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceNamespaceClient.java:466-469`
compares this normalized value, but retains the original `table_uri` for
opening.
`fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceCatalogClient.java:621-625`
joins `/tree/dev` before `?` only, producing
`s3://bucket/t.lance#anchor/tree/dev`. The namespace version check at
`fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceCatalogClient.java:696-701`
uses
`fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceManifestPaths.java:74-94`,
which strips the fragment and accepts the dev manifest. The pinned Lance 12
provider extracts `url.path()`, so opening the joined URI addresses `/t.lance`,
not `/t.lance/tree/dev`. This PR removes the previous comparison that
intentionally preserved fragments.
**When it bites.** A supported REST namespace responds with
managed_versioning=true, location=`s3://bucket/t.lance`,
table_uri=`s3://bucket/t.lance#anchor`; dev and main both have version 3 with
different rows. Doris authorizes dev v3 but opens main v3. This is a concrete
response/input path, not a reproduced cluster result; providers that never
return fragments avoid this trigger.
**Suggested fix.** Reject fragments in the selected managed dataset URI
before branch joining. If fragment support is needed, fix branch URI
construction and FE/BE addressing together. Add a branch test with equal
version numbers and different rows. A small guard in `loadTableAccess` is:
```diff
---
a/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceNamespaceClient.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceNamespaceClient.java
@@
if (managed) {
+ if (datasetUri.indexOf('#') >= 0) {
+ throw new RuntimeException("Lance managed table URI must
not contain a fragment: " + tableId);
+ }
```
Pinned SDK evidence: [Lance 12 URI
parsing](https://github.com/lance-format/lance/blob/v12.0.0/rust/lance-io/src/object_store.rs#L516-L524),
[provider path
extraction](https://github.com/lance-format/lance/blob/v12.0.0/rust/lance-io/src/object_store/providers.rs#L52-L59).
### F-05 · Resolve the REST rename source to its remote identity
- **Severity**: Major
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java:304-307`
- **Category**: correctness / caches / parallel paths
```java
execute("Failed to rename Lance table " + dbName + "." + oldName + "
to " + newName,
client -> {
client.renameTable(dbName, oldName, newName);
return null;
```
**What is wrong.** REST rename fails for a table whose stored Doris name is
case-folded.
**Why it happens.**
`fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java:424-427` passes
local `getDbName()/getName()` through
`fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:1267`.
This method forwards them unchanged;
`fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceNamespaceClient.java:227-235`
constructs the remote ID directly. In contrast, DROP and column mutations use
the resolved table’s remote names.
**When it bites.** Set `lower_case_table_names=1` on a REST catalog
containing case-sensitive remote table `Events`. Discovery exposes local
`events` and retains remoteName=`Events`; `ALTER TABLE catalog.default.events
RENAME renamed_events` nevertheless sends `[parent..., events]`. The request
fails despite resolving a valid source table. Explicit alias mapping is not
assumed: Lance inherits identity defaults for
fromRemoteTableName/fromRemoteDatabaseName.
**Suggested fix.** Resolve source DB/table before the SDK request,
preserving caller/local identity for cache hooks and edit log. Leave newName as
the requested new remote identifier. For example:
```diff
---
a/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java
@@
if (!catalog.isRestCatalogConfigured()) {
throw new DdlException("RENAME TABLE is not supported for Lance
filesystem catalogs");
}
+ ExternalDatabase<?> db = catalog.getDbNullable(dbName);
+ ExternalTable source = db == null ? null :
db.getTableNullable(oldName);
+ if (source == null) {
+ throw new DdlException("Failed to resolve Lance rename source:
" + dbName + "." + oldName);
+ }
execute("Failed to rename Lance table " + dbName + "." + oldName +
" to " + newName,
client -> {
- client.renameTable(dbName, oldName, newName);
+ client.renameTable(source.getRemoteDbName(),
source.getRemoteName(), newName);
```
### F-06 · Retire a reused rename destination on leader and follower
- **Severity**: Major
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java:315-317`
- **Category**: correctness / caches / parallel paths
```java
if (db.isPresent()) {
db.get().unregisterTable(oldName);
db.get().resetMetaCacheNames();
```
**What is wrong.** A successful REST rename can leave the destination’s
previous table object and schema visible.
**Why it happens.** Only oldName is retired.
`fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:476-478`
returns a resident object before reloading names, while its `resetNames` only
clears the name snapshot. URI-cache invalidation does not evict destination
schema.
`fe/fe-core/src/main/java/org/apache/doris/catalog/RefreshManager.java:263-267`
repeats the source-only retirement during replay; ordinary cold-source fallback
invalidates database schema, so that branch is not claimed to retain stale DESC.
**When it bites.** Warm `source(id INT)` and `destination(old STRING)` on an
FE. An external client deletes destination; Doris then renames source to
destination, which the remote namespace accepts. DESC destination still returns
old STRING. MVCC query snapshots can reload fresh schema, so this finding does
not claim every SELECT is stale. Rename validation does not veto the cached
destination. A warm follower is affected too.
**Suggested fix.** Use replay-safe retirement for both source and
destination, including schema, routed caches and row-count fencing. Update
refresh-log rename replay to retire the destination on warm-source replay as
well; preserve cold-source fallback cleanup. A leader-only extra unregister is
insufficient. Cover reuse with distinct schemas on two FEs.
### F-07 · Preserve explicit COMMENT flags on multi-column ADD
- **Severity**: Major
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java:346-350`
- **Category**: correctness / caches / parallel paths
```java
for (Column column : columns) {
validateAddColumn(column, null);
entries.add(new AddColumnsEntry()
.name(column.getName())
.expression(LanceTypeConverter.toAddColumnExpression(column.getType())));
```
**What is wrong.** Multi-column ADD silently loses requested column comments
instead of applying the advertised rejection.
**Why it happens.**
`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AddColumnsOp.java:103-106`
uses `translateToCatalogStyle`, which copies comment text but never sets
`Column.commentSpecified`.
`fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java:478-480`
rejects only that flag. This loop therefore passes validation, and its
AddColumnsEntry carries only name/expression. Single-column ADD uses
`translateToCatalogStyleForSchemaChange`, which preserves the flag.
**When it bites.** `ALTER TABLE t ADD COLUMN (x INT NULL COMMENT 'abc', y
INT NULL)` on a writable Lance namespace sends a successful ADD request and
creates x without its comment. The corresponding single-column statement is
rejected. No upstream batch-column validation rejects this comment.
**Suggested fix.** Use the schema-change conversion for batch ADD so
comment/default/nullability intent reaches connector validation. Add a
parser-to-command batch negative test that verifies no remote mutation. The
conversion below matches single ADD, but also changes realDefaultValue from
cooked to raw for other connectors, which was not validated here. A narrower
alternative is Lance command validation of AddColumnsOp
ColumnDefinition.isCommentSpecified. The conversion fix is:
```diff
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AddColumnsOp.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AddColumnsOp.java
@@
- Column col = columnDef.translateToCatalogStyle();
+ Column col = columnDef.translateToCatalogStyleForSchemaChange();
```
--
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]
