Copilot commented on code in PR #16858: URL: https://github.com/apache/lucene/pull/16858#discussion_r4314368527
########## 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); Review Comment: This exception message doesn’t indicate the allowed values, which makes misconfiguration harder to diagnose. Consider including the supported values in the message (e.g., `Unknown join.update.side='<value>' (expected: products|skus|both)`). ########## lucene/benchmark/src/test/org/apache/lucene/benchmark/byTask/tasks/TestJoinSearchTask.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.util.Properties; +import org.apache.lucene.benchmark.BenchmarkTestCase; +import org.apache.lucene.benchmark.byTask.PerfRunData; +import org.apache.lucene.benchmark.byTask.feeds.JoinDocMaker; +import org.apache.lucene.benchmark.byTask.utils.Config; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.store.FSDirectory; + +public class TestJoinSearchTask extends BenchmarkTestCase { + + private static final int PRODUCTS = 300, SKUS_PER_PRODUCT = 4; + + public void testAlgorithmsAgreeBeforeAndAfterUpdates() throws Exception { + for (String layout : new String[] {"products", "skus", "mixed", "block"}) { + execBenchmark( + new String[] { + "doc.maker=" + JoinDocMaker.class.getName(), + "join.layout=" + layout, + "join.products=" + PRODUCTS, + "join.skusPerProduct=" + SKUS_PER_PRODUCT, + "work.dir=" + getWorkDirPath() + "/" + layout, + "directory=FSDirectory", + "max.buffered=" + (50 + random().nextInt(500)), + "CreateIndex", + "[ { AddJoinDocs > : * ] : 2", + "CloseIndex", + }); + } + assertNumDocs("products", PRODUCTS); + assertNumDocs("skus", PRODUCTS * SKUS_PER_PRODUCT); + assertNumDocs("mixed", PRODUCTS * (SKUS_PER_PRODUCT + 1)); + assertNumDocs("block", PRODUCTS * (SKUS_PER_PRODUCT + 1)); + + long base; + try (PerfRunData runData = runData("both")) { + base = assertAlgorithmsAgree(runData); + } + for (String side : new String[] {"products", "skus", "both"}) { + try (PerfRunData runData = runData(side)) { + for (String group : new String[] {"split", "mixed", "block"}) { + JoinUpdateTask update = new JoinUpdateTask(runData); + update.setParams(group); + for (int i = 0; i < 3; i++) { + update.runAndMaybeStats(false); + } + } + assertNotEquals(side, base, assertAlgorithmsAgree(runData)); Review Comment: The assertion `assertNotEquals(side, base, ...)` can be flaky: randomized updates might not change the aggregated total hit count for the chosen randomized query sequence, even though updates were applied correctly. To make the test deterministic, consider verifying updates via a query that is guaranteed to be affected by a known updated doc/version (e.g., update a specific product/sku id and assert a targeted count changes), or drop the `assertNotEquals` and instead validate only (1) algorithms agree and (2) reopening with rollback restores the original totals. ########## .vscode/settings.json: ########## @@ -1,6 +1,17 @@ { "java.server.launchMode": "Standard", + "jdk.jdkhome": "/home/mikhail-khludnev/.jdks/jdk-25.0.3+9", + "jdk.project.jdkhome": "/home/mikhail-khludnev/.jdks/jdk-25.0.3+9", + "java.jdt.ls.java.home": "/home/mikhail-khludnev/.jdks/jdk-25.0.3+9", + "java.configuration.runtimes": [ + { + "name": "JavaSE-25", + "path": "/home/mikhail-khludnev/.jdks/jdk-25.0.3+9", + "default": true + } + ], "java.import.gradle.enabled": false, + "java.import.gradle.java.home": "/home/mikhail-khludnev/.jdks/jdk-25.0.3+9", Review Comment: These settings hardcode a user-specific absolute JDK path and a specific local JDK build, which will break other developers/CI and is generally not suitable for a shared repo. Suggestion: remove the per-user JDK path entries from the committed workspace settings (or move them to a local-only file via documented setup), and keep only portable settings (e.g., formatter URL) that apply across environments. ########## 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(); + return batch; Review Comment: For `mixed` and `block` groups, `productIndex` and `skuIndex` resolve to the same underlying index/manager, so this can trigger redundant `maybeRefreshBlocking()` calls (and potentially double-count refresh costs in benchmarks). Also, when `join.update.side` updates only products or only skus (in the `split` group), refreshing the untouched side adds avoidable work and can skew the intended measurement. Suggestion: refresh only the affected manager(s), and if both managers are the same instance, refresh once. -- 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]
