github-actions[bot] commented on code in PR #66637: URL: https://github.com/apache/doris/pull/66637#discussion_r3781698221
########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceIndexMetadataLoader.java: ########## @@ -0,0 +1,247 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.datasource.lance; + +import org.apache.doris.persist.gson.GsonUtils; + +import com.google.common.collect.ImmutableSet; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonToken; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.commons.lang3.StringUtils; +import org.lance.Dataset; +import org.lance.index.IndexDescription; +import org.lance.schema.LanceField; + +import java.io.IOException; +import java.io.StringReader; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.OptionalLong; +import java.util.Set; +import java.util.TreeMap; + +/** Loads and normalizes logical index metadata from one latest Lance dataset snapshot. */ +public final class LanceIndexMetadataLoader { + private static final int MAX_LOGICAL_INDEXES = 256; + private static final int MAX_COLUMNS_PER_INDEX = 64; + private static final int MAX_COLUMN_NAMES_BYTES = 16 * 1024; + private static final int MAX_EXTERNAL_STRING_BYTES = 1024; + private static final int MAX_PROPERTIES_BYTES = 400; + + private static final Set<String> PROPERTY_ALLOWLIST = ImmutableSet.of( + "metric_type", + "target_partition_size", + "compression_type", + "num_bits", + "num_sub_vectors", + "hnsw_max_connections", + "hnsw_construction_ef", + "hnsw_max_level"); + + private LanceIndexMetadataLoader() { + } + + /** Loads logical indexes and schema fields from the same latest dataset snapshot. */ + public static List<LanceLogicalIndex> load(String datasetUri, + Map<String, String> javaStorageOptions, BufferAllocator allocator) throws Exception { + try (Dataset dataset = Dataset.open().allocator(allocator).uri(datasetUri) + .readOptions(LanceReadOptions.build(javaStorageOptions, OptionalLong.empty())).build()) { + Map<Integer, String> topLevelFieldNames = new HashMap<>(); + for (LanceField field : dataset.getLanceSchema().fields()) { + topLevelFieldNames.put(field.getId(), field.getName()); + } + return normalize(dataset.describeIndices(), topLevelFieldNames); Review Comment: [P2] Handle valid Lance system indexes before describing all entries. In the exact pinned SDK, optimized/MemWAL datasets can contain `__lance_frag_reuse` or `__mem_wal` with empty fields. `describe_indices(None)` includes them, but JNI unconditionally renders each entry's details and these system detail types have no scalar plugin, so this call aborts before any user index is returned; the local empty-field check would reject them after an SDK fix as well. Please pin/use a conversion that handles system entries and deliberately exclude them from SHOW INDEX, with a system-plus-user-index regression. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowIndexCommand.java: ########## @@ -108,30 +112,58 @@ private ShowResultSet handleShowIndex(ConnectContext ctx, StmtExecutor executor) analyze(ctx); List<List<String>> rows = Lists.newArrayList(); - // in show index, only support internal catalog - DatabaseIf db = Env.getCurrentEnv().getCatalogMgr() - .getCatalogOrAnalysisException(tableNameInfo.getCtl()) - .getDbOrAnalysisException(tableNameInfo.getDb()); + CatalogIf catalog = Env.getCurrentEnv().getCatalogMgr() + .getCatalogOrAnalysisException(tableNameInfo.getCtl()); + DatabaseIf db = catalog.getDbOrAnalysisException(tableNameInfo.getDb()); Review Comment: [P2] Gate REST catalogs before database resolution. On a cache miss this call populates the external meta-cache by listing Lance namespaces, and table resolution can then list remote tables; the REST-unsupported check is not reached until `loadTableIndexMetadata`. An unavailable REST endpoint therefore returns an unknown-database/provider failure after unnecessary I/O instead of the fixed unsupported error promised by this change. After the privilege check, reject the locally configured REST type before resolving the database/table, and cover the route with an unreachable namespace. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceIndexMetadataLoader.java: ########## @@ -0,0 +1,247 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.datasource.lance; + +import org.apache.doris.persist.gson.GsonUtils; + +import com.google.common.collect.ImmutableSet; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonToken; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.commons.lang3.StringUtils; +import org.lance.Dataset; +import org.lance.index.IndexDescription; +import org.lance.schema.LanceField; + +import java.io.IOException; +import java.io.StringReader; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.OptionalLong; +import java.util.Set; +import java.util.TreeMap; + +/** Loads and normalizes logical index metadata from one latest Lance dataset snapshot. */ +public final class LanceIndexMetadataLoader { + private static final int MAX_LOGICAL_INDEXES = 256; + private static final int MAX_COLUMNS_PER_INDEX = 64; + private static final int MAX_COLUMN_NAMES_BYTES = 16 * 1024; + private static final int MAX_EXTERNAL_STRING_BYTES = 1024; + private static final int MAX_PROPERTIES_BYTES = 400; + + private static final Set<String> PROPERTY_ALLOWLIST = ImmutableSet.of( Review Comment: [P1] Parse the SDK's nested vector details. In the pinned Lance version, IVF-PQ details are serialized with a nested `compression` object, and HNSW settings are nested under `hnsw`; none of the flattened keys in this allowlist are emitted. Consequently those objects are silently dropped—the new regression already shows only `metric_type` although its fixture creates PQ with 8 bits and 2 sub-vectors. Please parse a bounded allowlist within the actual nested objects and test with the SDK-shaped JSON. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceIndexMetadataLoader.java: ########## @@ -0,0 +1,247 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.datasource.lance; + +import org.apache.doris.persist.gson.GsonUtils; + +import com.google.common.collect.ImmutableSet; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonToken; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.commons.lang3.StringUtils; +import org.lance.Dataset; +import org.lance.index.IndexDescription; +import org.lance.schema.LanceField; + +import java.io.IOException; +import java.io.StringReader; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.OptionalLong; +import java.util.Set; +import java.util.TreeMap; + +/** Loads and normalizes logical index metadata from one latest Lance dataset snapshot. */ +public final class LanceIndexMetadataLoader { + private static final int MAX_LOGICAL_INDEXES = 256; + private static final int MAX_COLUMNS_PER_INDEX = 64; + private static final int MAX_COLUMN_NAMES_BYTES = 16 * 1024; + private static final int MAX_EXTERNAL_STRING_BYTES = 1024; + private static final int MAX_PROPERTIES_BYTES = 400; + + private static final Set<String> PROPERTY_ALLOWLIST = ImmutableSet.of( + "metric_type", + "target_partition_size", + "compression_type", + "num_bits", + "num_sub_vectors", + "hnsw_max_connections", + "hnsw_construction_ef", + "hnsw_max_level"); + + private LanceIndexMetadataLoader() { + } + + /** Loads logical indexes and schema fields from the same latest dataset snapshot. */ + public static List<LanceLogicalIndex> load(String datasetUri, + Map<String, String> javaStorageOptions, BufferAllocator allocator) throws Exception { + try (Dataset dataset = Dataset.open().allocator(allocator).uri(datasetUri) + .readOptions(LanceReadOptions.build(javaStorageOptions, OptionalLong.empty())).build()) { + Map<Integer, String> topLevelFieldNames = new HashMap<>(); + for (LanceField field : dataset.getLanceSchema().fields()) { Review Comment: [P1] Resolve valid nested field IDs. Lance indexes can target nested fields, and `describeIndices()` returns the indexed leaf ID, but this map contains only schema roots, so normalization later rejects the ID and fails SHOW INDEX for the entire table. The pinned Lance tag exposes `LanceField.getChildren()` and constructs canonical escaped paths from field ancestry (including names containing dots). Please recurse the schema into a complete ID-to-path map and replace the negative nested-ID test with a real nested-index case. -- 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]
