Copilot commented on code in PR #6595: URL: https://github.com/apache/hive/pull/6595#discussion_r3557015709
########## standalone-metastore/metastore-search/src/main/java/org/apache/hive/search/config/SearchConfig.java: ########## @@ -0,0 +1,86 @@ +/* + * 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.hive.search.config; + +import java.time.Duration; + +import org.apache.hadoop.conf.Configuration; + +public record SearchConfig(Configuration configuration) { + public static final String REFRESH_INTERVAL_SECONDS = "metastore.search.refresh.interval.seconds"; + public static final long REFRESH_INTERVAL_SECONDS_DEFAULT = 1L; + + public static final String DEFAULT_LIMIT = "metastore.search.default.limit"; + public static final int DEFAULT_LIMIT_DEFAULT = 10; + + public static final String INIT_READY_TIMEOUT_MS = "metastore.search.init.ready.timeout.ms"; + public static final long INIT_READY_TIMEOUT_MS_DEFAULT = 300L; + + public static final String HYBRID_SEMANTIC_WEIGHT = "metastore.search.hybrid.semantic.weight"; + public static final float HYBRID_SEMANTIC_WEIGHT_DEFAULT = 0.4f; + Review Comment: Tests and other new code reference `SearchConfig.HYBRID_MATCH_WEIGHT` / `HYBRID_MATCH_WEIGHT_DEFAULT`, but these constants are not defined (only semantic weight is). This will fail compilation and prevents configuring match-vs-semantic weighting consistently. ########## standalone-metastore/metastore-search/src/main/java/org/apache/hive/search/config/SearchConfig.java: ########## @@ -0,0 +1,86 @@ +/* + * 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.hive.search.config; + +import java.time.Duration; + +import org.apache.hadoop.conf.Configuration; + +public record SearchConfig(Configuration configuration) { + public static final String REFRESH_INTERVAL_SECONDS = "metastore.search.refresh.interval.seconds"; + public static final long REFRESH_INTERVAL_SECONDS_DEFAULT = 1L; + + public static final String DEFAULT_LIMIT = "metastore.search.default.limit"; + public static final int DEFAULT_LIMIT_DEFAULT = 10; + + public static final String INIT_READY_TIMEOUT_MS = "metastore.search.init.ready.timeout.ms"; + public static final long INIT_READY_TIMEOUT_MS_DEFAULT = 300L; + + public static final String HYBRID_SEMANTIC_WEIGHT = "metastore.search.hybrid.semantic.weight"; + public static final float HYBRID_SEMANTIC_WEIGHT_DEFAULT = 0.4f; + + public static final String FUSION_PRIOR = "metastore.search.fusion.prior"; + public static final float FUSION_PRIOR_DEFAULT = 0.5f; + + public static final String BAYESIAN_SAMPLES = "metastore.search.bayesian.samples"; + public static final int BAYESIAN_SAMPLES_DEFAULT = 100; + + public static final String BAYESIAN_TOKENS_PER_QUERY = "metastore.search.bayesian.tokens.per.query"; + public static final int BAYESIAN_TOKENS_PER_QUERY_DEFAULT = 5; + + public static final String BAYESIAN_SEED = "metastore.search.bayesian.seed"; + public static final long BAYESIAN_SEED_DEFAULT = 42L; + + public Duration getRefreshInterval() { + return Duration.ofSeconds( + configuration.getLong(REFRESH_INTERVAL_SECONDS, REFRESH_INTERVAL_SECONDS_DEFAULT)); + } + + public int getDefaultLimit() { + return configuration.getInt(DEFAULT_LIMIT, DEFAULT_LIMIT_DEFAULT); + } + + public long getInitReadyTimeoutMs() { + return configuration.getLong(INIT_READY_TIMEOUT_MS, INIT_READY_TIMEOUT_MS_DEFAULT); + } + + public float getHybridSemanticWeight() { + float weight = configuration.getFloat(HYBRID_SEMANTIC_WEIGHT, HYBRID_SEMANTIC_WEIGHT_DEFAULT); + if (weight >= 1 || weight <= 0) { + throw new IllegalArgumentException("metastore.search.hybrid.semantic.weight " + + "should be configured as between 0.0 and 1.0"); + } + return weight; + } Review Comment: `SearchConfig` is missing `getHybridMatchWeight()` (used by new tests and hybrid fusion logic). Also, having separate match/semantic weights risks inconsistent configuration; it’s safer if the getters treat them as complements that always sum to 1. ########## standalone-metastore/metastore-search/src/main/java/org/apache/hive/search/search/HybridSearch.java: ########## @@ -0,0 +1,187 @@ +/* + * 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.hive.search.search; + +import java.util.List; +import java.util.Map; + +import org.apache.commons.lang3.StringUtils; +import org.apache.hive.search.config.SearchConfig; +import org.apache.hive.search.exception.SearchException; +import org.apache.hive.search.mapping.FieldSchema; +import org.apache.hive.search.mapping.IndexMapping; +import org.apache.hive.search.metastore.MetastoreTableMapper; + +/** Builds RRF queries that fuse lexical and semantic retrieval on hybrid fields. */ +public final class HybridSearch { + private HybridSearch() {} + + public record ParsedHybridQuery( + String field, + String queryText, + Float semanticWeight) {} Review Comment: `ParsedHybridQuery` only defines `(field, queryText, semanticWeight)`, but the added tests and call sites expect both `matchWeight()` and `semanticWeight()` (and even construct it with 4 args). As-is, this will not compile and also drops the parsed `match_weight` value from requests. ########## standalone-metastore/metastore-search/src/main/java/org/apache/hive/search/mapping/field/Field.java: ########## @@ -0,0 +1,23 @@ +/* + * 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.hive.search.mapping.field; + +/** A single field value within a {@lin}. */ Review Comment: The Javadoc has a broken inline tag (`{@lin}`), which will break Javadoc generation and doesn’t point readers to the intended type. ########## standalone-metastore/metastore-search/src/main/java/org/apache/hive/search/search/HybridSearch.java: ########## @@ -0,0 +1,187 @@ +/* + * 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.hive.search.search; + +import java.util.List; +import java.util.Map; + +import org.apache.commons.lang3.StringUtils; +import org.apache.hive.search.config.SearchConfig; +import org.apache.hive.search.exception.SearchException; +import org.apache.hive.search.mapping.FieldSchema; +import org.apache.hive.search.mapping.IndexMapping; +import org.apache.hive.search.metastore.MetastoreTableMapper; + +/** Builds RRF queries that fuse lexical and semantic retrieval on hybrid fields. */ +public final class HybridSearch { + private HybridSearch() {} + + public record ParsedHybridQuery( + String field, + String queryText, + Float semanticWeight) {} + + public static ParsedHybridQuery parse(Object hybridBody, IndexMapping mapping) + throws SearchException { + if (hybridBody instanceof String queryText) { + String field = requireHybridField(mapping, null); + return new ParsedHybridQuery(field, queryText, null); + } + if (!(hybridBody instanceof Map<?, ?> body)) { + throw new SearchException("hybrid query must be a string or object"); + } + @SuppressWarnings("unchecked") + Map<String, Object> hybridMap = (Map<String, Object>) body; + + HybridWeights weights = parseHybridWeights(hybridMap); + Review Comment: `parseHybridWeights(hybridMap)` can throw `IllegalArgumentException` (from `HybridWeights.validate()`), which leaks out of `parse(...)` despite the method contract declaring `throws SearchException`. This will produce unexpected unchecked failures for invalid user input instead of a `SearchException`. ########## standalone-metastore/metastore-search/src/main/java/org/apache/hive/search/index/store/IndexBackupUtils.java: ########## @@ -0,0 +1,121 @@ +/* + * 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.hive.search.index.store; + +import java.io.IOException; +import java.io.InputStream; +import java.net.URI; +import java.util.List; +import java.util.Optional; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hive.search.config.IndexStateConfig; +import org.apache.hive.search.exception.IndexIOException; +import org.apache.hive.search.index.manifest.IndexManifest; + +public final class IndexBackupUtils { + private IndexBackupUtils() {} + + /** Push local index state to the backup (remote) location. */ + public static boolean syncToBackup(IndexStateClient local, IndexStateClient remote) + throws IOException { + Optional<IndexManifest> localManifest = local.readManifest(); + if (localManifest.isEmpty()) { + return false; + } + Optional<IndexManifest> remoteManifest = remote.readManifest(); + if (remoteManifest.isPresent() + && remoteManifest.get().lastEventId() >= localManifest.get().lastEventId()) { + return false; + } + applyDiff(local, remote, localManifest.get().diff(remoteManifest.orElse(null))); + return remote.writeManifest(localManifest.get()); + } + + /** + * If a previous restore left a staging manifest, finalize it when files already match, + * or keep it in place so {@link #restoreFromBackup} can resume. + */ + public static void resolveInterruptedRestore(IndexStateClient local) + throws IOException { + Optional<IndexManifest> staging = local.readStagingManifest(); + if (staging.isEmpty()) { + local.clearStagingManifest(); + return; + } Review Comment: `resolveInterruptedRestore()` clears the staging manifest whenever `readStagingManifest()` returns empty. Since `LocalStateClient.readStagingManifest()` returns `Optional.empty()` on I/O read failure (not just when the file is absent), this can delete the staging manifest and prevent a restore from resuming after a transient read error. ########## standalone-metastore/metastore-search/src/main/java/org/apache/hive/search/search/SearchReqResp.java: ########## @@ -0,0 +1,55 @@ +/* + * 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.hive.search.search; + +import java.util.List; +import java.util.Map; + +import org.apache.hive.search.exception.SearchException; + +public final class SearchReqResp { + /** Public search mode exposed on the Metastore Thrift API. */ + public enum Mode { + KEYWORD, + SEMANTIC, + HYBRID + } + + public record Request( + Map<String, Object> query, + List<String> fields, + int size, + String catalogName, + String databaseName) { + public Request { + query = query == null ? Map.of() : Map.copyOf(query); + fields = fields == null ? List.of() : List.copyOf(fields); + } + + public static Request validated(Map<String, Object> query, List<String> fields, int size, + String catalogName, String databaseName) throws SearchException { + if (size < 0) { + throw new SearchException("size must be non-negative"); + } + return new Request(query, fields, size, catalogName, databaseName); + } + } + + public record Response(List<Map<String, Object>> hits, long total) {} Review Comment: `SearchReqResp.Response` doesn’t defensively copy `hits`, unlike `Request`. Since this is part of the public request/response surface, callers can mutate the provided list after construction, breaking immutability assumptions. -- 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]
