Copilot commented on code in PR #16858: URL: https://github.com/apache/lucene/pull/16858#discussion_r4317209245
########## lucene/benchmark/src/java/org/apache/lucene/benchmark/byTask/tasks/JoinSearchTask.java: ########## @@ -0,0 +1,172 @@ +/* + * 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.lucene.benchmark.byTask.tasks; + +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.BRAND; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.BRANDS; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.COLOR; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.COLORS; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.FK; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.FK_NUM; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.ID; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.ID_NUM; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.PRODUCT; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.SIZE; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.SIZES; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.TYPE; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.value; + +import java.util.ArrayList; +import java.util.List; +import java.util.Random; +import java.util.TreeSet; +import org.apache.lucene.benchmark.byTask.PerfRunData; +import org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker; +import org.apache.lucene.index.SortedDocValues; +import org.apache.lucene.index.Term; +import org.apache.lucene.search.BooleanClause.Occur; +import org.apache.lucene.search.BooleanQuery; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.SearcherManager; +import org.apache.lucene.search.TermInSetQuery; +import org.apache.lucene.search.TermQuery; +import org.apache.lucene.search.join.BitSetProducer; +import org.apache.lucene.search.join.JoinUtil; +import org.apache.lucene.search.join.QueryBitSetProducer; +import org.apache.lucene.search.join.ScoreMode; +import org.apache.lucene.search.join.ToParentBlockJoinQuery; +import org.apache.lucene.util.BytesRef; + +/** + * Counts products joined from skus, over the indices written by {@link JoinDocMaker} (see + * conf/join-index.alg). Each query selects skus by a few colors or a few sizes, joins them to their + * products and filters products by a few brands. Every instance draws the same query sequence from + * <b>join.seed</b>, so instances with different algorithms are comparable. + * + * <p>The parameter is the algorithm: + * + * <ul> + * <li><b>term</b> - skus' {@link SortedDocValues} FK to products' PK terms (products, skus) + * <li><b>numeric</b> - skus' {@link org.apache.lucene.index.NumericDocValues} FK to products' PK + * points (products, skus) + * <li><b>globalOrd</b> - global ordinals over {@link SortedDocValues} FK (mixed) + * <li><b>block</b> - {@link ToParentBlockJoinQuery} (block) + * </ul> + * + * <p>Other config properties: <b>join.dir</b> and <b>join.searchThreads</b>, see {@link + * JoinIndices}; <b>join.scoreMode</b> - Default: None. + */ +public class JoinSearchTask extends PerfTask { + + private static final BitSetProducer PRODUCTS = + new QueryBitSetProducer(new TermQuery(new Term(TYPE, PRODUCT))); + + private final Random random; + private final ScoreMode scoreMode; + private String algorithm; + private SearcherManager fromManager; + private SearcherManager toManager; + private IndexSearcher from; + private IndexSearcher to; + private long totalHits; + + public JoinSearchTask(PerfRunData runData) { + super(runData); + random = new Random(runData.getConfig().get("join.seed", 42)); + scoreMode = ScoreMode.valueOf(runData.getConfig().get("join.scoreMode", "None")); + } + + @Override + public void setup() throws Exception { + super.setup(); + if (fromManager == null) { + JoinIndices indices = JoinIndices.get(getRunData()); + String layout = algorithm.equals("globalOrd") ? "mixed" : algorithm; + boolean split = algorithm.equals("term") || algorithm.equals("numeric"); + fromManager = indices.index(split ? "skus" : layout).manager(); + toManager = indices.index(split ? "products" : layout).manager(); + } + from = fromManager.acquire(); + to = toManager.acquire(); + } + + @Override + public int doLogic() throws Exception { + boolean byColor = random.nextBoolean(); + Query skus = terms(byColor ? COLOR : SIZE, byColor ? COLORS : SIZES, 2 + random.nextInt(3)); + Query brands = terms(BRAND, BRANDS, 1 + random.nextInt(3)); + Query join = + switch (algorithm) { + case "term" -> JoinUtil.createJoinQuery(FK, false, ID, skus, from, scoreMode); + case "numeric" -> + JoinUtil.createJoinQuery(FK_NUM, false, ID_NUM, Long.class, skus, from, scoreMode); + case "globalOrd" -> + JoinUtil.createJoinQuery( + FK, + skus, + new TermQuery(new Term(TYPE, PRODUCT)), + from, + scoreMode, + ((JoinIndices.JoinSearcher) from).ordinalMap()); + default -> new ToParentBlockJoinQuery(skus, PRODUCTS, scoreMode); Review Comment: The `default` branch silently treats any unexpected `algorithm` value as the block-join implementation. Since `setParams` validates supported algorithms, it’s safer and clearer to handle `"block"` explicitly and make the `default` branch throw (or otherwise fail fast). This prevents surprising behavior if `algorithm` is ever set incorrectly (e.g., via config or future refactors bypassing `setParams`). ########## lucene/benchmark/src/java/org/apache/lucene/benchmark/byTask/tasks/JoinIndices.java: ########## @@ -0,0 +1,150 @@ +/* + * 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.lucene.benchmark.byTask.tasks; + +import java.io.Closeable; +import java.io.IOException; +import java.nio.file.Path; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.ThreadFactory; +import org.apache.lucene.benchmark.byTask.PerfRunData; +import org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.IndexWriter; +import org.apache.lucene.index.IndexWriterConfig; +import org.apache.lucene.index.IndexWriterConfig.OpenMode; +import org.apache.lucene.index.MultiDocValues; +import org.apache.lucene.index.OrdinalMap; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.SearcherFactory; +import org.apache.lucene.search.SearcherManager; +import org.apache.lucene.store.Directory; +import org.apache.lucene.store.FSDirectory; +import org.apache.lucene.util.IOUtils; +import org.apache.lucene.util.NamedThreadFactory; + +/** + * Writers and NRT searchers over the join indices at <b>join.dir</b>/{layout}/index (Default: + * work/join), shared by join tasks and closed with {@link PerfRunData}. Updates are never + * committed, so the indices stay intact for the next run. + * + * <p><b>join.searchThreads</b> - size of the executor shared by all searchers, i.e. segment + * parallel search on both join sides. Fixed per run, since searchers are only recreated on refresh. + * Default: 0, search segments sequentially. + */ +final class JoinIndices implements Closeable { + + /** + * Searcher that carries the global ordinals of {@link JoinDocMaker#FK} for the mixed index. They + * are built by the first query on each new searcher, so a cold search after refresh pays for + * them. + */ + static final class JoinSearcher extends IndexSearcher { + private final boolean ordinals; + private OrdinalMap ordinalMap; + private boolean built; + + JoinSearcher(IndexReader reader, Executor executor, boolean ordinals) { + super(reader, executor); + this.ordinals = ordinals; + } + + synchronized OrdinalMap ordinalMap() throws IOException { + if (ordinals && !built) { + if (MultiDocValues.getSortedValues(getIndexReader(), JoinDocMaker.FK) + instanceof MultiDocValues.MultiSortedDocValues multi) { + ordinalMap = multi.mapping; // a single segment needs no ordinal map + } + built = true; + } + return ordinalMap; + } + } + + record Index(Directory dir, IndexWriter writer, SearcherManager manager) {} + + private final Path root; + private final ExecutorService executor; + private final Map<String, Index> indices = new HashMap<>(); + + private JoinIndices(Path root, int searchThreads) { + this.root = root; + ThreadFactory named = new NamedThreadFactory("join-search"); + // daemon, since Benchmark never closes PerfRunData, and the pool must not keep the JVM alive + executor = + searchThreads > 0 + ? Executors.newFixedThreadPool( + searchThreads, + r -> { + Thread thread = named.newThread(r); + thread.setDaemon(true); + return thread; + }) + : null; + } + + static JoinIndices get(PerfRunData runData) { + synchronized (runData) { + JoinIndices indices = (JoinIndices) runData.getPerfObject(JoinIndices.class.getName()); + if (indices == null) { + indices = + new JoinIndices( + Path.of(runData.getConfig().get("join.dir", "work/join")), + runData.getConfig().get("join.searchThreads", 0)); + runData.setPerfObject(JoinIndices.class.getName(), indices); + } + return indices; + } + } + + synchronized Index index(String layout) throws IOException { + Index index = indices.get(layout); + if (index == null) { + Directory dir = FSDirectory.open(root.resolve(layout).resolve("index")); + IndexWriter writer = + new IndexWriter( + dir, new IndexWriterConfig().setOpenMode(OpenMode.APPEND).setCommitOnClose(false)); + boolean ordinals = layout.equals("mixed"); + SearcherManager manager = + new SearcherManager( + writer, + new SearcherFactory() { + @Override + public IndexSearcher newSearcher(IndexReader reader, IndexReader previous) { + return new JoinSearcher(reader, executor, ordinals); + } + }); + index = new Index(dir, writer, manager); + indices.put(layout, index); + } + return index; + } + + @Override + public synchronized void close() throws IOException { + for (Index index : indices.values()) { + IOUtils.close(index.manager(), index.writer()::rollback, index.dir()); + } + if (executor != null) { + executor.shutdown(); + } Review Comment: `IOUtils.close(...)` typically accepts `Closeable` instances; `index.writer()::rollback` is a method reference that does not implement `Closeable` and is likely to fail compilation. Also, rollback ordering matters (close/release the `SearcherManager` first, then rollback the writer, then close the directory). Suggest restructuring this block to: close the manager, call `index.writer().rollback()` explicitly, and then close the directory (with appropriate exception handling). ########## lucene/benchmark/src/java/org/apache/lucene/benchmark/byTask/tasks/JoinUpdateTask.java: ########## @@ -0,0 +1,158 @@ +/* + * 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.lucene.benchmark.byTask.tasks; + +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.ID; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.PID; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.productId; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.skuId; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Random; +import org.apache.lucene.benchmark.byTask.PerfRunData; +import org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker; +import org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.Dataset; +import org.apache.lucene.index.IndexWriter; +import org.apache.lucene.index.Term; + +/** + * Overrides a batch of random existing products and/or skus with new versions of their values (see + * {@link JoinDocMaker}), then refreshes the {@link JoinIndices} searchers, so it measures update + * plus NRT reopen. Data size and seed come from the {@link Dataset} recorded in the index at + * indexing time. + * + * <p>The parameter is the layout group: <b>split</b> (products and skus indices), <b>mixed</b> or + * <b>block</b>. The block layout rewrites the whole product block on any update, i.e. suffers + * update amplification. All groups draw the same update sequence from <b>join.seed</b>, so after + * the same number of updates all join algorithms agree. + * + * <p>Other config properties: <b>join.update.side</b> - products | skus | both. Default: both; + * <b>join.update.batch</b> - updates per task run. Default: 100. + */ +public class JoinUpdateTask extends PerfTask { + + /** Update sequence and the latest version of each updated doc, per layout group. */ + private static final class Versions { + final Random random; + final Map<Long, Long> products = new HashMap<>(); + final Map<Long, Long> skus = new HashMap<>(); + long version; + + Versions(long seed) { + random = new Random(seed); + } + } + + private final String side; + private final int batch; + private String group; + private JoinIndices.Index productIndex; + private JoinIndices.Index skuIndex; + private Versions versions; + private Dataset docs; + + public JoinUpdateTask(PerfRunData runData) { + super(runData); + side = runData.getConfig().get("join.update.side", "both"); + batch = runData.getConfig().get("join.update.batch", 100); + if (!List.of("products", "skus", "both").contains(side)) { + throw new IllegalArgumentException("join.update.side: " + side); + } + } + + @Override + public void setup() throws Exception { + super.setup(); + if (versions == null) { + PerfRunData runData = getRunData(); + JoinIndices indices = JoinIndices.get(runData); + productIndex = indices.index(group.equals("split") ? "products" : group); + skuIndex = indices.index(group.equals("split") ? "skus" : group); + docs = Dataset.fromUserData(productIndex.writer().getLiveCommitData()); + Dataset skus = Dataset.fromUserData(skuIndex.writer().getLiveCommitData()); + if (!docs.equals(skus)) { + throw new IllegalStateException("products " + docs + " but skus " + skus); + } + synchronized (runData) { + String key = JoinUpdateTask.class.getName() + "." + group; + versions = (Versions) runData.getPerfObject(key); + if (versions == null) { + versions = new Versions(runData.getConfig().get("join.seed", 42)); + runData.setPerfObject(key, versions); + } + } + } + } + + @Override + public int doLogic() throws Exception { + String productLayout = group.equals("split") ? "products" : group; + String skuLayout = group.equals("split") ? "skus" : group; + IndexWriter productWriter = productIndex.writer(); + IndexWriter skuWriter = skuIndex.writer(); + boolean products = !side.equals("skus"); + boolean skus = !side.equals("products"); + synchronized (versions) { + for (int i = 0; i < batch; i++) { + long product = versions.random.nextLong(docs.products()); + long sku = product * docs.skusPerProduct() + versions.random.nextInt(docs.skusPerProduct()); + long version = ++versions.version; + if (products) { + versions.products.put(product, version); + } + if (skus) { + versions.skus.put(sku, version); + } + if (group.equals("block")) { + productWriter.updateDocuments( + new Term(PID, productId(product)), + docs.block( + product, + versions.products.getOrDefault(product, 0L), + s -> versions.skus.getOrDefault(s, 0L))); + continue; + } + if (products) { + productWriter.updateDocument( + new Term(ID, productId(product)), docs.product(productLayout, product, version)); + } + if (skus) { + skuWriter.updateDocument(new Term(ID, skuId(sku)), docs.sku(skuLayout, sku, version)); + } + } + } + productIndex.manager().maybeRefreshBlocking(); + skuIndex.manager().maybeRefreshBlocking(); Review Comment: For non-split groups (`mixed` and `block`), `productIndex` and `skuIndex` appear to reference the same underlying index/manager, which makes this refresh run twice unnecessarily. Consider refreshing only once when both managers are the same (e.g., compare references) to avoid redundant work on each update batch. ########## lucene/benchmark/src/java/org/apache/lucene/benchmark/byTask/tasks/JoinUpdateTask.java: ########## @@ -0,0 +1,158 @@ +/* + * 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.lucene.benchmark.byTask.tasks; + +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.ID; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.PID; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.productId; +import static org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.skuId; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Random; +import org.apache.lucene.benchmark.byTask.PerfRunData; +import org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker; +import org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker.Dataset; +import org.apache.lucene.index.IndexWriter; +import org.apache.lucene.index.Term; + +/** + * Overrides a batch of random existing products and/or skus with new versions of their values (see + * {@link JoinDocMaker}), then refreshes the {@link JoinIndices} searchers, so it measures update + * plus NRT reopen. Data size and seed come from the {@link Dataset} recorded in the index at + * indexing time. + * + * <p>The parameter is the layout group: <b>split</b> (products and skus indices), <b>mixed</b> or + * <b>block</b>. The block layout rewrites the whole product block on any update, i.e. suffers + * update amplification. All groups draw the same update sequence from <b>join.seed</b>, so after + * the same number of updates all join algorithms agree. + * + * <p>Other config properties: <b>join.update.side</b> - products | skus | both. Default: both; + * <b>join.update.batch</b> - updates per task run. Default: 100. + */ +public class JoinUpdateTask extends PerfTask { + + /** Update sequence and the latest version of each updated doc, per layout group. */ + private static final class Versions { + final Random random; + final Map<Long, Long> products = new HashMap<>(); + final Map<Long, Long> skus = new HashMap<>(); + long version; + + Versions(long seed) { + random = new Random(seed); + } + } + + private final String side; + private final int batch; + private String group; + private JoinIndices.Index productIndex; + private JoinIndices.Index skuIndex; + private Versions versions; + private Dataset docs; + + public JoinUpdateTask(PerfRunData runData) { + super(runData); + side = runData.getConfig().get("join.update.side", "both"); + batch = runData.getConfig().get("join.update.batch", 100); + if (!List.of("products", "skus", "both").contains(side)) { + throw new IllegalArgumentException("join.update.side: " + side); + } + } + + @Override + public void setup() throws Exception { + super.setup(); + if (versions == null) { + PerfRunData runData = getRunData(); + JoinIndices indices = JoinIndices.get(runData); + productIndex = indices.index(group.equals("split") ? "products" : group); + skuIndex = indices.index(group.equals("split") ? "skus" : group); Review Comment: Using `group.equals("split")` will throw an NPE if `group` is ever unset when `setup()` runs. Swapping to the null-safe form (`"split".equals(group)`) avoids that failure mode and makes the code more robust to lifecycle/order changes in how tasks are initialized. (Same pattern appears elsewhere with `algorithm.equals(...)`.) -- 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]
