Copilot commented on code in PR #13215:
URL: https://github.com/apache/ignite/pull/13215#discussion_r3372474841
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/GridDistributedLockRequest.java:
##########
@@ -250,45 +257,74 @@ public boolean skipStore() {
* @param skipReadThrough Skip read-through cache store flag.
*/
Review Comment:
The Javadoc for `skipReadThrough(boolean)` still says "Sets skip store flag
value", but this method controls the skip *read-through* flag. This is
confusing when reading/maintaining the flags logic.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheOperationContext.java:
##########
@@ -108,38 +98,26 @@ public CacheOperationContext(
this.recovery = recovery;
this.readRepairStrategy = readRepairStrategy;
this.appAttrs = appAttrs;
+ this.calciteEngineCall = calciteEngineCall;
}
/**
- * @return Keep binary flag.
+ * Helper.
*/
- public boolean isKeepBinary() {
- return keepBinary;
+ public static CacheOperationContext instance() {
+ return new Builder().build();
}
Review Comment:
`CacheOperationContext.instance()` currently builds a new context on every
call, so it is not an actual reusable default instance despite the name. Since
`CacheOperationContext` is immutable, this should return a cached static
instance to avoid repeated allocations and make intent clear.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/GridDistributedLockRequest.java:
##########
@@ -250,45 +257,74 @@ public boolean skipStore() {
* @param skipReadThrough Skip read-through cache store flag.
*/
private void skipReadThrough(boolean skipReadThrough) {
- flags = skipReadThrough ? (byte)(flags | SKIP_READ_THROUGH_FLAG_MASK)
: (byte)(flags & ~SKIP_READ_THROUGH_FLAG_MASK);
+ setFlag(skipReadThrough, SKIP_READ_THROUGH_FLAG_MASK);
}
/**
* @return Skip store flag.
*/
Review Comment:
The Javadoc for `skipReadThrough()` incorrectly says it returns the skip
store flag; it should describe the skip read-through flag to match the method
semantics.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheOperationContext.java:
##########
@@ -151,214 +129,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();
}
/**
- * @param recovery Recovery flag.
- * @return New instance of CacheOperationContext with recovery flag.
+ * @return Skip store.
*/
- public CacheOperationContext setRecovery(boolean recovery) {
- return new CacheOperationContext(
- skipStore,
- skipReadThrough,
- keepBinary,
- expiryPlc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- appAttrs);
+ public boolean skipStore() {
+ return skipStore;
}
- /**
- * @param readRepairStrategy Read Repair strategy.
- * @return New instance of CacheOperationContext with Read Repair flag.
- */
- public CacheOperationContext setReadRepairStrategy(ReadRepairStrategy
readRepairStrategy) {
- return new CacheOperationContext(
- skipStore,
- skipReadThrough,
- keepBinary,
- expiryPlc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- appAttrs);
+ /** Context with skipStore flag. */
+ public CacheOperationContext withSkipStore() {
+ return builder(this).skipStore(true).build();
}
/**
- * @param appAttrs Application attributes.
- * @return New instance of CacheOperationContext with application
attributes.
+ * @return Skip read-through cache store.
*/
- public CacheOperationContext setApplicationAttributes(Map<String, String>
appAttrs) {
- return new CacheOperationContext(
- skipStore,
- skipReadThrough,
- keepBinary,
- expiryPlc,
- noRetries,
- dataCenterId,
- recovery,
- readRepairStrategy,
- new HashMap<>(appAttrs));
+ public boolean skipReadThrough() {
+ return skipReadThrough;
}
- /**
- * @return Partition recover flag.
- */
- public boolean recovery() {
- return recovery;
+ /** Context with {@link CacheOperationContext#skipReadThrough} flag. */
+ public CacheOperationContext withSkipReadThrough() {
+ return builder(this).skipReadThrough(true).build();
+ }
+
+ /** @return Calcite engine execution flag. */
+ public boolean calciteEngine() {
+ return calciteEngineCall;
+ }
+
+ /** Context with {@link CacheOperationContext#calciteEngine} flag. */
+ public CacheOperationContext withCalciteEngine() {
+ return builder(this).calciteEngine(true).build();
}
/**
- * @return Read Repair strategy.
+ * @return {@link ExpiryPolicy} associated with this projection.
*/
- public ReadRepairStrategy readRepairStrategy() {
- return readRepairStrategy;
+ @Nullable public ExpiryPolicy expiry() {
+ return expiryPlc;
+ }
+
+ /** Context with {@link CacheOperationContext#expiryPlc}. */
+ public CacheOperationContext withExpiryPolicy(ExpiryPolicy plc) {
+ return builder(this).expiryPolicy(plc).build();
+ }
+
+ /** {@inheritDoc} */
+ @Override public String toString() {
+ return S.toString(CacheOperationContext.class, this);
}
/**
- * @return No retries flag.
+ * Creates the builder for cache operations context.
+ *
+ * @return Builder for cache operations context.
*/
- public boolean noRetries() {
- return noRetries;
+ public static Builder builder(CacheOperationContext ctx) {
+ return new Builder(ctx);
}
/**
- * @return Application attributes.
+ * Creates the builder from existing context.
+ *
+ * @return Builder for cache operations context.
*/
- public Map<String, String> applicationAttributes() {
- return appAttrs;
+ public static Builder builder() {
+ return new Builder();
}
- /** {@inheritDoc} */
- @Override public String toString() {
- return S.toString(CacheOperationContext.class, this);
+ /** Cache operations context builder. */
+ public static class Builder {
+ /** Skip store. */
+ @GridToStringInclude
+ private boolean skipStore;
+
+ /** Skip read through. */
+ @GridToStringInclude
+ private boolean skipReadThrough;
+
+ /** No retries flag. */
+ @GridToStringInclude
+ private boolean noRetries;
+
+ /** Recovery flag. */
+ private boolean recovery;
+
+ /** Read-repair strategy. */
+ private ReadRepairStrategy readRepairStrategy;
+
+ /** Keep binary flag. */
+ private boolean keepBinary;
+
+ /** Expiry policy. */
+ private ExpiryPolicy expiryPlc;
+
+ /** Data center Id. */
+ private Byte dataCenterId;
+
+ /** Application attributes. */
+ private Map<String, String> appAttrs;
+
+ /** Calcite engine execution flag. */
+ private boolean calciteEngine;
+
+ /** */
+ Builder() {
+ // No context.
+ }
+
+ /** */
+ Builder(CacheOperationContext ctx) {
+ skipStore = ctx.skipStore;
+ skipReadThrough = ctx.skipReadThrough;
+ noRetries = ctx.noRetries;
+ recovery = ctx.recovery;
+ readRepairStrategy = ctx.readRepairStrategy;
+ keepBinary = ctx.keepBinary;
+ expiryPlc = ctx.expiryPlc;
+ dataCenterId = ctx.dataCenterId;
+ appAttrs = ctx.appAttrs;
+ calciteEngine = ctx.calciteEngineCall;
+ }
+
+ /**
+ * CacheOperationContext with keepBinary flag.
+ *
+ * @see IgniteInternalCache#keepBinary()
+ */
+ public Builder keepBinary(boolean keepBinary) {
+ this.keepBinary = keepBinary;
+ return this;
+ }
+
+ /**
+ * CacheOperationContext with skipStore flag.
+ *
+ * @see IgniteInternalCache#skipStore()
+ */
Review Comment:
The `Builder#skipStore(boolean)` Javadoc links to
`IgniteInternalCache#skipStore()`, which is a getter; the enabling API is
`withSkipStore()`. This makes the Javadoc misleading for users searching for
the corresponding cache API.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/PublicApiIntegrationTest.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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);
+ }
Review Comment:
When `transactional` is true, the transaction is started but not
closed/rolled back if an exception happens during the loop. Other Calcite
integration tests typically use try-with-resources for `Transaction` to
guarantee cleanup; this test should follow the same pattern to avoid leaking
transaction state on failures.
--
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]
