Copilot commented on code in PR #13215:
URL: https://github.com/apache/ignite/pull/13215#discussion_r3372809009
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheOperationContext.java:
##########
@@ -151,214 +132,291 @@ public CacheOperationContext keepBinary() {
return dataCenterId;
}
- /**
- * @return Skip store.
- */
- public boolean skipStore() {
- return skipStore;
+ /** Context with dataCenterId. */
+ public CacheOperationContext dataCenterId(Byte dataCenterId) {
+ return builder(this).dataCenterId(dataCenterId).build();
}
/**
- * See {@link IgniteInternalCache#setSkipStore(boolean)}.
- *
- * @param skipStore Skip store flag.
- * @return New instance of CacheOperationContext with skip store flag.
+ * @return Partition recover flag.
*/
- public CacheOperationContext setSkipStore(boolean skipStore) {
- return new CacheOperationContext(
- skipStore,
- skipReadThrough,
- keepBinary,
- expiryPlc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- appAttrs);
+ public boolean recovery() {
+ return recovery;
}
- /** @return Skip read-through cache store. */
- public boolean skipReadThrough() {
- return skipReadThrough;
+ /** Context with recovery flag. */
+ public CacheOperationContext withRecovery() {
+ return builder(this).recovery(true).build();
}
/**
- * See {@link IgniteInternalCache#withApplicationAttributes(Map)}.
- *
- * @return New instance of CacheOperationContext with new application
attributes.
+ * @return Read Repair strategy.
*/
- public CacheOperationContext withApplicationAttributes(Map<String, String>
attrs) {
- return new CacheOperationContext(
- skipStore,
- skipReadThrough,
- keepBinary,
- expiryPlc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- Collections.unmodifiableMap(attrs));
+ @Nullable public ReadRepairStrategy readRepairStrategy() {
+ return readRepairStrategy;
}
- /**
- * See {@link IgniteInternalCache#withSkipReadThrough()}.
- *
- * @return New instance of CacheOperationContext with skip store flag.
- */
- public CacheOperationContext withSkipReadThrough() {
- return new CacheOperationContext(
- skipStore,
- true,
- keepBinary,
- expiryPlc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- appAttrs);
+ /** Context with read repair strategy. */
+ public CacheOperationContext readRepairStrategy(ReadRepairStrategy
strategy) {
+ return builder(this).readRepairStrategy(strategy).build();
}
/**
- * @return {@link ExpiryPolicy} associated with this projection.
+ * @return No retries flag.
*/
- @Nullable public ExpiryPolicy expiry() {
- return expiryPlc;
+ public boolean noRetries() {
+ return noRetries;
}
- /**
- * See {@link IgniteInternalCache#withExpiryPolicy(ExpiryPolicy)}.
- *
- * @param plc {@link ExpiryPolicy} to associate with this projection.
- * @return New instance of CacheOperationContext with skip store flag.
- */
- public CacheOperationContext withExpiryPolicy(ExpiryPolicy plc) {
- return new CacheOperationContext(
- skipStore,
- skipReadThrough,
- keepBinary,
- plc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- appAttrs);
+ /** Context with noRetries flag. */
+ public CacheOperationContext withNoRetries() {
+ return builder(this).noRetries(true).build();
}
/**
- * @param noRetries No retries flag.
- * @return Operation context.
+ * @return Application attributes.
*/
- public CacheOperationContext setNoRetries(boolean noRetries) {
- return new CacheOperationContext(
- skipStore,
- skipReadThrough,
- keepBinary,
- expiryPlc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- appAttrs);
+ @Nullable public Map<String, String> applicationAttributes() {
+ return appAttrs;
}
- /**
- * @param dataCenterId Data center id.
- * @return Operation context.
- */
- public CacheOperationContext setDataCenterId(byte dataCenterId) {
- return new CacheOperationContext(
- skipStore,
- skipReadThrough,
- keepBinary,
- expiryPlc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- appAttrs);
+ /** Context with application attributes. */
+ public CacheOperationContext withApplicationAttributes(Map<String, String>
attrs) {
+ return
builder(this).applicationAttributes(Collections.unmodifiableMap(attrs)).build();
Review Comment:
`withApplicationAttributes` wraps `attrs` with
`Collections.unmodifiableMap(...)` before passing it to the builder, but
`Builder#applicationAttributes(...)` already defensively copies and wraps the
map. The extra wrapping is redundant work and makes the intent less clear.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheOperationContext.java:
##########
@@ -151,214 +132,291 @@ public CacheOperationContext keepBinary() {
return dataCenterId;
}
- /**
- * @return Skip store.
- */
- public boolean skipStore() {
- return skipStore;
+ /** Context with dataCenterId. */
+ public CacheOperationContext dataCenterId(Byte dataCenterId) {
+ return builder(this).dataCenterId(dataCenterId).build();
}
/**
- * See {@link IgniteInternalCache#setSkipStore(boolean)}.
- *
- * @param skipStore Skip store flag.
- * @return New instance of CacheOperationContext with skip store flag.
+ * @return Partition recover flag.
*/
- public CacheOperationContext setSkipStore(boolean skipStore) {
- return new CacheOperationContext(
- skipStore,
- skipReadThrough,
- keepBinary,
- expiryPlc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- appAttrs);
+ public boolean recovery() {
+ return recovery;
}
- /** @return Skip read-through cache store. */
- public boolean skipReadThrough() {
- return skipReadThrough;
+ /** Context with recovery flag. */
+ public CacheOperationContext withRecovery() {
+ return builder(this).recovery(true).build();
}
/**
- * See {@link IgniteInternalCache#withApplicationAttributes(Map)}.
- *
- * @return New instance of CacheOperationContext with new application
attributes.
+ * @return Read Repair strategy.
*/
- public CacheOperationContext withApplicationAttributes(Map<String, String>
attrs) {
- return new CacheOperationContext(
- skipStore,
- skipReadThrough,
- keepBinary,
- expiryPlc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- Collections.unmodifiableMap(attrs));
+ @Nullable public ReadRepairStrategy readRepairStrategy() {
+ return readRepairStrategy;
}
- /**
- * See {@link IgniteInternalCache#withSkipReadThrough()}.
- *
- * @return New instance of CacheOperationContext with skip store flag.
- */
- public CacheOperationContext withSkipReadThrough() {
- return new CacheOperationContext(
- skipStore,
- true,
- keepBinary,
- expiryPlc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- appAttrs);
+ /** Context with read repair strategy. */
+ public CacheOperationContext readRepairStrategy(ReadRepairStrategy
strategy) {
+ return builder(this).readRepairStrategy(strategy).build();
}
Review Comment:
Similar to `dataCenterId(Byte)`, the setter-style overload
`readRepairStrategy(ReadRepairStrategy)` is easy to misread as the getter and
is inconsistent with the rest of the `withXxx()` fluent API. Adding a
`withReadRepairStrategy(ReadRepairStrategy)` alias would improve readability
and consistency.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheOperationContext.java:
##########
@@ -151,214 +132,291 @@ public CacheOperationContext keepBinary() {
return dataCenterId;
}
- /**
- * @return Skip store.
- */
- public boolean skipStore() {
- return skipStore;
+ /** Context with dataCenterId. */
+ public CacheOperationContext dataCenterId(Byte dataCenterId) {
+ return builder(this).dataCenterId(dataCenterId).build();
}
Review Comment:
The setter-style overload `dataCenterId(Byte)` is easy to confuse with the
getter `dataCenterId()` and is inconsistent with the other `withXxx()` methods
in this class. Consider adding a `withDataCenterId(Byte)` alias to make the
fluent API clearer (and potentially migrate call sites over time).
##########
modules/benchmarks/src/main/java/org/apache/ignite/internal/benchmarks/jmh/sql/JmhSqlInsertBenchmark.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * 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.ignite.internal.benchmarks.jmh.sql;
+
+import java.util.concurrent.TimeUnit;
+import java.util.stream.IntStream;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.runner.Runner;
+import org.openjdk.jmh.runner.options.Options;
+import org.openjdk.jmh.runner.options.OptionsBuilder;
+
+import static java.lang.String.format;
+import static java.util.stream.Collectors.joining;
+
+/**
+ * Benchmark for insertion operation, comparing SQL APIs.
+ */
+@State(Scope.Benchmark)
+@Fork(1)
+@Threads(1)
+@Warmup(iterations = 10, time = 2)
+@Measurement(iterations = 10, time = 2)
+@BenchmarkMode(Mode.AverageTime)
+@OutputTimeUnit(TimeUnit.MICROSECONDS)
+public class JmhSqlInsertBenchmark extends JmhSqlAbstractBenchmark {
+ /** */
+ private int id;
+
+ /** */
+ private static final String FIELD_VAL = "a".repeat(100);
+
+ /** */
+ private static final String TABLE_NAME = "dept";
+
+ /** */
+ private String insertStr;
+
+ /** */
+ private String multiInsertStr;
+
+ /**
+ * Initiate new tables.
+ */
+ @Override public void setup() {
+ super.setup();
+
+ insertStr = createInsertStatement();
+ multiInsertStr = createMultiInsertStatement();
+
+ executeSql("CREATE TABLE " + TABLE_NAME +
+ "(ycsb_key int PRIMARY KEY," +
+ "field1 varchar(100)," +
+ "field2 varchar(100)," +
+ "field3 varchar(100)," +
+ "field4 varchar(100)," +
+ "field5 varchar(100)," +
+ "field6 varchar(100)," +
+ "field7 varchar(100)," +
+ "field8 varchar(100)," +
+ "field9 varchar(100)," +
+ "field10 varchar(100))"
+ );
+ }
+
+ /**
+ * Benchmark for SQL insert via embedded client.
+ */
+ @Benchmark
+ public void sqlSimpleInsert() {
+ executeSql(insertStr, id++);
+ }
+
+ /**
+ * Benchmark for batch SQL insert via embedded client.
+ */
+ @Benchmark
+ public void sqlBatchInsert() {
+ id += 2;
+
+ executeSql(multiInsertStr, id, id + 1);
+ }
Review Comment:
`sqlBatchInsert` increments `id` before using it, which makes the inserted
key sequence start at 2 and is harder to reason about. Incrementing after the
insert keeps the method consistent with `sqlSimpleInsert` and makes key
progression clearer.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/PublicApiIntegrationTest.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * 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.ignite.internal.processors.query.calcite.integration;
+
+import java.util.List;
+import org.apache.ignite.IgniteCache;
+import org.apache.ignite.cache.CacheAtomicityMode;
+import org.apache.ignite.cache.query.SqlFieldsQuery;
+import org.apache.ignite.calcite.CalciteQueryEngineConfiguration;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.SqlConfiguration;
+import org.apache.ignite.transactions.Transaction;
+import org.junit.Test;
+
+import static
org.apache.ignite.transactions.TransactionConcurrency.PESSIMISTIC;
+import static
org.apache.ignite.transactions.TransactionIsolation.READ_COMMITTED;
+
+/** Public api integration tests. */
+public class PublicApiIntegrationTest extends AbstractBasicIntegrationTest {
+ /** */
+ @Override protected IgniteConfiguration getConfiguration(String
igniteInstanceName) throws Exception {
+ IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
+
+ cfg.setSqlConfiguration(new SqlConfiguration()
+ .setQueryEnginesConfiguration(new
CalciteQueryEngineConfiguration().setDefault(true)));
+
+ return cfg;
+ }
+
+ /** */
+ @Test
+ public void testSimpleInsert() {
+ IgniteCache<Object, Object> cache =
client.createCache(DEFAULT_CACHE_NAME);
+
+ runQuery(0, nodeCount() * 10, false, cache);
+
+ cache = cache.withKeepBinary();
+
+ runQuery(nodeCount() * 10, 2 * nodeCount() * 10, false, cache);
+
+ List<List<?>> res = cache.query(new SqlFieldsQuery("SELECT * FROM
emp")).getAll();
+
+ assertEquals("Unexpected result set size: " + res.size(), 1,
res.size());
+ }
+
+ /** */
+ @Test
+ public void testTxInsert() {
+ CacheConfiguration<Object, Object> ccfg = new
CacheConfiguration<>(DEFAULT_CACHE_NAME);
+ ccfg.setAtomicityMode(CacheAtomicityMode.TRANSACTIONAL);
+
+ IgniteCache<?, ?> cache = client.createCache(ccfg);
+
+ runQuery(0, nodeCount() * 10, true, cache);
+
+ cache = cache.withKeepBinary();
+
+ runQuery(nodeCount() * 10, 2 * nodeCount() * 10, true, cache);
+
+ List<List<?>> res = cache.query(new SqlFieldsQuery("SELECT * FROM
emp")).getAll();
+
+ assertEquals("Unexpected result set size: " + res.size(), 1,
res.size());
+ }
+
+ /** */
+ private void runQuery(int begin, int end, boolean transactional,
IgniteCache<?, ?> cache) {
+ Transaction tx = null;
+
+ cache.query(new SqlFieldsQuery("CREATE TABLE IF NOT EXISTS emp(empid
INTEGER, deptid INTEGER, name VARCHAR, salary INTEGER, " +
+ "PRIMARY KEY(empid, deptid)) WITH \"AFFINITY_KEY=deptid" +
(transactional ? ", ATOMICITY=transactional" : "") + "\""))
+ .getAll();
+
+ if (transactional) {
+ //noinspection resource
+ tx = client.transactions().txStart(PESSIMISTIC, READ_COMMITTED);
+ }
+
+ try {
+ for (int i = begin; i < end; i++) {
+ cache.query(new SqlFieldsQuery("INSERT INTO emp (empid,
deptid, name, salary) VALUES (?, ?, ?, ?)").setArgs(
+ i, i % 2, "Employee " + i, i)).getAll();
+
+ cache.query(new SqlFieldsQuery("UPDATE emp SET name = '' WHERE
empid = ? AND deptid = ?").setArgs(i, i % 2)).getAll();
+ cache.query(new SqlFieldsQuery("DELETE FROM emp WHERE empid =
?").setArgs(i - 1)).getAll();
+
+ cache.query(new SqlFieldsQuery(
+ "MERGE INTO emp dst USING table(system_range(1, 1000)) src
ON dst.salary = src.x " +
+ "WHEN MATCHED THEN UPDATE SET dst.salary =
src.x")).getAll();
+ }
+ }
+ finally {
+ if (transactional)
+ tx.commit();
+ }
Review Comment:
The transaction is committed from a `finally` block, which means it will be
committed even if an exception is thrown during the DML loop. This can mask the
original failure (commit may throw) and can leave partially applied changes
committed when the test is supposed to fail. Prefer try-with-resources and
commit only on the success path (letting close/rollback happen on exceptions).
--
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]
