Skip to content

Commit de4daa4

Browse files
u70b3u70b3
committed
[fix](lance) Fail the IF preflight closed on column collisions and representation mismatches
Three admission-preflight gaps, each closed on the same pinned snapshot: The definition match compared the request column's raw parser spelling against the logical side's loader path segment, which backtick-escapes field names containing characters outside [A-Za-z0-9_] (embedded backticks doubled). Columns such as "a.b" or "c d" were therefore falsely rejected as a different definition; the request column is now formatted into the same path-segment representation before both sides pass name normalization. An omitted request num_bits is semantically 8 - the validator accepts the omission and the persisted job pins 8 - but the preflight skipped the property entirely, so a snapshot exposing compression.num_bits=4 produced a false no-op. The request side now defaults to 8 before the exposed value is compared. A dataset can hold top-level fields that differ only by case (V and v), and the table column lookup returns the first equalsIgnoreCase hit, so CREATE/REPLACE could journal the wrong stored column. Admission now fails closed when the snapshot matches more than one top-level field under that exact lookup relation, before any column resolution.
1 parent 4fba663 commit de4daa4

3 files changed

Lines changed: 171 additions & 21 deletions

File tree

‎fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceIndexAdmission.java‎

Lines changed: 54 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import com.google.gson.JsonElement;
3838
import com.google.gson.JsonObject;
3939
import com.google.gson.JsonParser;
40+
import org.lance.schema.LanceField;
4041

4142
import java.util.ArrayList;
4243
import java.util.HashMap;
@@ -57,7 +58,8 @@
5758
* first, before target capture and the snapshot read (fail cheap-first — a reserved name
5859
* rejected at admission depth costs no remote read), then case-only collision analysis, IF
5960
* preflight (including the two-stage {@code matches}: requested-algorithm equality plus
60-
* physical-family corroboration), schema contract from the stored column name, locator
61+
* physical-family corroboration), column-lookup collision analysis, schema contract from
62+
* the stored column name, locator
6163
* normalization, deterministic properties JSON, positive-quota assertion, and only then exactly
6264
* one id allocation and the durable {@code createJob} transfer.
6365
* Every rejection before {@code createJob} leaves no job, no fence, no quota charge, no journal
@@ -147,24 +149,29 @@ static Outcome admitCreate(SnapshotLoader loader, LanceExternalCatalog catalog,
147149
}
148150
return catalogMgr.withLanceIndexAdmission(catalog, target, () -> new Outcome(null));
149151
}
150-
// 5. Schema contract v1 from the stored column name (never the raw user spelling).
152+
// 5. Fail closed when the table lookup relation cannot resolve the request column
153+
// uniquely: a dataset can hold top-level fields that differ only by case (V versus v),
154+
// and ExternalTable.getColumn returns the first hit, so admitting would journal an
155+
// arbitrary one of them. The pinned snapshot decides, never the cached table schema.
156+
rejectIfAmbiguousLookupColumn(snapshot, def.getCols().get(0));
157+
// 6. Schema contract v1 from the stored column name (never the raw user spelling).
151158
String storedColumnName = storedColumnName(table, def.getCols().get(0));
152159
LanceIndexSchemaContract contract =
153160
LanceSchemaContractBuilder.build(snapshot.getTopLevelFields(), storedColumnName);
154-
// 6. The fence locator is the normalized dataset uri of the same pinned snapshot.
161+
// 7. The fence locator is the normalized dataset uri of the same pinned snapshot.
155162
String locator = normalizeLocator(snapshot);
156-
// 7. Deterministic normalized properties JSON for ANN; scalar families persist null.
163+
// 8. Deterministic normalized properties JSON for ANN; scalar families persist null.
157164
boolean ann = def.getLanceIndexType() == null;
158165
String indexType = ann ? annIndexType(def) : def.getLanceIndexType();
159166
String propertiesJson = ann ? buildAnnPropertiesJson(def) : null;
160167
// D7 backstop: quota values from fe.conf bypass the ADMIN SET callback, so admission
161168
// re-asserts positivity before any id allocation or durable transfer.
162169
assertPositiveQuotas();
163170
return catalogMgr.withLanceIndexAdmission(catalog, target, () -> {
164-
// 8. Exactly one id allocation, after every preflight above has passed.
171+
// 9. Exactly one id allocation, after every preflight above has passed.
165172
long jobId = Env.getCurrentEnv().getNextId();
166173
String creator = ConnectContext.get().getQualifiedUser();
167-
// 9. REPLACE on an existing name persists the stored display name (section 4.1) so the
174+
// 10. REPLACE on an existing name persists the stored display name (section 4.1) so the
168175
// worker locates the case-sensitive target; a fresh REPLACE keeps the user's spelling.
169176
String persistedDisplayName = (orReplace && storedName != null) ? storedName : displayName;
170177
LanceIndexJob job;
@@ -181,7 +188,7 @@ static Outcome admitCreate(SnapshotLoader loader, LanceExternalCatalog catalog,
181188
Config.lance_index_job_max_unresolved_per_table,
182189
Config.lance_index_job_max_unresolved_per_catalog,
183190
Config.lance_index_job_max_unresolved_global);
184-
// 10. The job and its fence are durable once createJob returns.
191+
// 11. The job and its fence are durable once createJob returns.
185192
return new Outcome(jobId);
186193
});
187194
}
@@ -246,9 +253,11 @@ static Outcome admitDrop(SnapshotLoader loader, LanceExternalCatalog catalog,
246253
* stored logical algorithm under family normalization — a same-name different-algorithm
247254
* request is a mismatch, never a no-op; (b) the physical entry of the same name must exist
248255
* and back the logical algorithm (snapshot self-consistency, failing closed); (c) the single
249-
* normalized column must be equal; (d) whitelist properties are compared per property — a
256+
* normalized column, with the request side in the same loader path-segment representation as
257+
* the logical side, must be equal; (d) whitelist properties are compared per property — a
250258
* value the request sets and the snapshot exposes must be equal, an unexposed snapshot value
251-
* is skipped, and a property the request omits is never compared.
259+
* is skipped, and a property the request omits is never compared except num_bits, whose
260+
* omitted request value defaults to the always-persisted 8.
252261
*/
253262
private static boolean matchesExistingDefinition(LanceIndexAdmissionSnapshot snapshot,
254263
String storedName, IndexDefinition def) {
@@ -281,7 +290,12 @@ private static boolean matchesExistingDefinition(LanceIndexAdmissionSnapshot sna
281290
if (logical.getColumns().size() != 1) {
282291
return false;
283292
}
284-
String requestColumn = LanceIndexNameNormalizer.normalize(def.getCols().get(0));
293+
// The logical column is a loader path segment: field names containing characters outside
294+
// [A-Za-z0-9_] are backtick-escaped (embedded backticks doubled). The parser hands over the
295+
// raw spelling, so the request column is formatted with the same rule before both sides
296+
// pass name normalization — otherwise such columns are falsely rejected as a mismatch.
297+
String requestColumn = LanceIndexNameNormalizer.normalize(
298+
LanceIndexMetadataLoader.formatFieldPathSegment(def.getCols().get(0)));
285299
if (!LanceIndexNameNormalizer.normalize(logical.getColumns().get(0)).equals(requestColumn)) {
286300
return false;
287301
}
@@ -290,9 +304,9 @@ private static boolean matchesExistingDefinition(LanceIndexAdmissionSnapshot sna
290304

291305
/**
292306
* Per-property whitelist comparison (metric ↔ metric_type, num_sub_vectors ↔
293-
* compression.num_sub_vectors, num_bits ↔ compression.num_bits). num_partitions is never
294-
* compared (section 2.2). BTREE/BITMAP carry no user build properties, so the comparison is
295-
* vacuous for them.
307+
* compression.num_sub_vectors, num_bits ↔ compression.num_bits; an omitted request num_bits
308+
* compares as the always-persisted 8). num_partitions is never compared (section 2.2).
309+
* BTREE/BITMAP carry no user build properties, so the comparison is vacuous for them.
296310
*/
297311
private static boolean whitelistPropertiesMatch(LanceLogicalIndex logical, IndexDefinition def) {
298312
if (def.getLanceIndexType() != null) {
@@ -327,8 +341,12 @@ private static boolean whitelistPropertiesMatch(LanceLogicalIndex logical, Index
327341
}
328342
JsonObject compression = exposed == null || !exposed.has("compression")
329343
? null : exposed.getAsJsonObject("compression");
344+
// The request side of num_bits is never "unset": the validator accepts an omitted value
345+
// and the persisted job pins it to 8 (section 2.4), so the preflight compares an
346+
// effective 8 against any exposed compression.num_bits instead of skipping.
347+
String requestNumBits = request.get("num_bits") == null ? "8" : request.get("num_bits");
330348
return numericPropertyMatches(request.get("num_sub_vectors"), compression, "num_sub_vectors")
331-
&& numericPropertyMatches(request.get("num_bits"), compression, "num_bits");
349+
&& numericPropertyMatches(requestNumBits, compression, "num_bits");
332350
}
333351

334352
/**
@@ -438,6 +456,28 @@ private static Map<String, String> normalizedAnnProperties(Map<String, String> p
438456
return normalized;
439457
}
440458

459+
/**
460+
* Fails closed when more than one top-level field of the pinned snapshot matches the request
461+
* column under the table lookup relation {@code String.equalsIgnoreCase} — deliberately not a
462+
* ROOT-lowercase fold, which decides some Unicode pairs differently and would diverge from
463+
* the relation {@link org.apache.doris.datasource.ExternalTable#getColumn} actually applies.
464+
* That lookup returns the first hit, so an ambiguous request could journal the wrong stored
465+
* column (design section 4.1 fail-closed rule, applied to columns).
466+
*/
467+
private static void rejectIfAmbiguousLookupColumn(LanceIndexAdmissionSnapshot snapshot,
468+
String requestColumn) throws AnalysisException {
469+
int matches = 0;
470+
for (LanceField field : snapshot.getTopLevelFields()) {
471+
if (requestColumn.equalsIgnoreCase(field.getName())) {
472+
++matches;
473+
}
474+
}
475+
if (matches > 1) {
476+
rejectInvalid("index column '" + requestColumn
477+
+ "' is ambiguous: multiple Lance fields differ only by case");
478+
}
479+
}
480+
441481
private static String storedColumnName(LanceExternalTable table, String requestColumn)
442482
throws AnalysisException {
443483
Column column = table.getColumn(requestColumn);

‎fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceIndexMetadataLoader.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,9 @@ private static void collectFieldNames(LanceField field, String parentPath, int d
336336
}
337337
}
338338

339-
private static String formatFieldPathSegment(String segment) {
339+
// Package-visible so admission formats request-side column names into the same path-segment
340+
// representation the logical side of the IF preflight carries (test-seam convention).
341+
static String formatFieldPathSegment(String segment) {
340342
boolean requiresQuoting = segment.codePoints()
341343
.anyMatch(codePoint -> !Character.isLetterOrDigit(codePoint)
342344
&& codePoint != '_');

0 commit comments

Comments
 (0)