Copilot commented on code in PR #12936:
URL: https://github.com/apache/ignite/pull/12936#discussion_r2994620317
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java:
##########
@@ -205,6 +208,13 @@ public class ExecutionServiceImpl<Row> extends
AbstractService implements Execut
/** */
private final Map<String, FragmentPlan> fragmentPlanCache = new
GridBoundedConcurrentLinkedHashMap<>(1024);
+ /**
+ * Transaction modified entries holder.
+ *
+ * @see TransactionConfiguration#isTxAwareQueriesEnabled()
+ */
+ private TxAwareModifiedEntriesHolder mofiedEntriesHolder;
Review Comment:
Identifier typo: `mofiedEntriesHolder` should be `modifiedEntriesHolder` (or
similar). Keeping the misspelling in a field name will propagate to many call
sites and makes searching/reading harder.
```suggestion
private TxAwareModifiedEntriesHolder modifiedEntriesHolder;
```
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionContext.java:
##########
@@ -145,11 +145,15 @@ public class ExecutionContext<Row> extends
AbstractQueryContext implements DataC
/** */
private Object[] correlations = new Object[16];
+ /** Modified entries holder. */
+ @Nullable private final TxAwareModifiedEntriesHolder mofiedEntriesHolder;
+
Review Comment:
Identifier typo: `mofiedEntriesHolder` should be `modifiedEntriesHolder` (or
similar). Since this is a field on `ExecutionContext`, the typo propagates into
constructor args and call sites and makes the API harder to use/search.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java:
##########
@@ -626,6 +641,8 @@ private ListFieldsQueryCursor<?> mapAndExecutePlan(
MemoryTracker qryMemoryTracker =
qry.createMemoryTracker(memoryTracker, cfg.getQueryMemoryQuota());
final GridNearTxLocal userTx = Commons.queryTransaction(qry.context(),
ctx.cache().context());
+ final Collection<QueryTxEntry> writeEntrs = userTx == null ?
+ mofiedEntriesHolder.retrieve() :
ExecutionContext.transactionChanges(userTx.writeEntries());
Review Comment:
Variable name typo: `writeEntrs` would be clearer as `writeEntries` (or
similar).
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/TxAwareModifiedEntriesHolder.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.exec;
+
+import java.util.Collection;
+import java.util.Collections;
+import
org.apache.ignite.internal.processors.query.calcite.message.QueryTxEntry;
+import org.jetbrains.annotations.Nullable;
+
+/** Per thread modified entries holder. */
+public class TxAwareModifiedEntriesHolder {
+ /** Transaction modified entries holder. */
+ @Nullable private final ThreadLocal<Collection<QueryTxEntry>> holder;
+
+ /** */
+ public TxAwareModifiedEntriesHolder(boolean txAware) {
+ if (txAware)
+ holder = new ThreadLocal<>();
+ else
+ holder = null;
+ }
+
+ /** Store entries if applicable. */
+ public void store(Collection<QueryTxEntry> items) {
+ if (holder != null)
+ holder.set(items);
+ }
+
+ /** Retirieve entries if applicable. */
Review Comment:
Typo in JavaDoc: "Retirieve" → "Retrieve".
```suggestion
/** Retrieve entries if applicable. */
```
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/TxAwareModifiedEntriesHolder.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.exec;
+
+import java.util.Collection;
+import java.util.Collections;
+import
org.apache.ignite.internal.processors.query.calcite.message.QueryTxEntry;
+import org.jetbrains.annotations.Nullable;
+
+/** Per thread modified entries holder. */
+public class TxAwareModifiedEntriesHolder {
+ /** Transaction modified entries holder. */
+ @Nullable private final ThreadLocal<Collection<QueryTxEntry>> holder;
+
+ /** */
+ public TxAwareModifiedEntriesHolder(boolean txAware) {
+ if (txAware)
+ holder = new ThreadLocal<>();
+ else
+ holder = null;
+ }
+
+ /** Store entries if applicable. */
+ public void store(Collection<QueryTxEntry> items) {
+ if (holder != null)
+ holder.set(items);
+ }
+
+ /** Retirieve entries if applicable. */
+ public Collection<QueryTxEntry> retrieve() {
+ if (holder != null)
+ return holder.get();
+ else
+ return Collections.emptyList();
Review Comment:
`retrieve()` can return `null` (when `holder != null` but nothing was stored
yet, or `store(null)` was called), but the method signature returns a non-null
`Collection<QueryTxEntry>`. This can lead to unexpected NPEs at call sites and
makes the API misleading. Consider returning `@Nullable
Collection<QueryTxEntry>` (and update callers accordingly) or normalize to
`Collections.emptyList()` when the thread-local value is `null` (and possibly
also when tx-aware mode is disabled) to keep the contract consistent.
```suggestion
if (holder == null)
return Collections.emptyList();
Collection<QueryTxEntry> entries = holder.get();
return entries != null ? entries : Collections.emptyList();
```
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java:
##########
@@ -901,7 +919,8 @@ private void onMessage(UUID nodeId, final QueryStartRequest
msg) {
createIoTracker(nodeId, msg.originatingQueryId()),
msg.timeout(),
Commons.parametersMap(msg.parameters()),
- msg.queryTransactionEntries()
+ msg.queryTransactionEntries(),
+ null
Review Comment:
The new `TxAwareModifiedEntriesHolder` parameter is passed as `null` when
creating `ExecutionContext` for remote fragments. That means
`ExecutionContext.execute()` on remote nodes will not populate the thread-local
with `msg.queryTransactionEntries()`, so nested queries started from UDFs on
those nodes won't be able to pick up tx-aware modified entries via
`mofiedEntriesHolder.retrieve()`. Consider passing the node's
`mofiedEntriesHolder` here instead of `null` so the behavior is consistent
between local and remote fragment execution.
```suggestion
modifiedEntriesHolder
```
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/UserDefinedTxAwareFunctionsIntegrationTest.java:
##########
@@ -0,0 +1,250 @@
+/*
+ * 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.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.IgniteCache;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.cache.QueryEntity;
+import org.apache.ignite.cache.query.SqlFieldsQuery;
+import org.apache.ignite.cache.query.annotations.QuerySqlFunction;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.util.tostring.GridToStringInclude;
+import org.apache.ignite.internal.util.typedef.internal.S;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.junits.WithSystemProperty;
+import org.apache.ignite.transactions.Transaction;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+import static org.apache.ignite.cache.CacheAtomicityMode.TRANSACTIONAL;
+import static org.apache.ignite.cache.CacheMode.PARTITIONED;
+import static
org.apache.ignite.internal.processors.query.calcite.CalciteQueryProcessor.IGNITE_CALCITE_USE_QUERY_BLOCKING_TASK_EXECUTOR;
+import static
org.apache.ignite.transactions.TransactionConcurrency.PESSIMISTIC;
+import static
org.apache.ignite.transactions.TransactionIsolation.READ_COMMITTED;
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.junit.Assert.assertThat;
+
+/**
+ * Integration test for user defined functions with tx aware.
+ */
+@WithSystemProperty(key = IGNITE_CALCITE_USE_QUERY_BLOCKING_TASK_EXECUTOR,
value = "true")
+public class UserDefinedTxAwareFunctionsIntegrationTest extends
AbstractBasicIntegrationTransactionalTest {
+ /** */
+ @Parameterized.Parameter()
+ public SqlTransactionMode sqlTxMode;
Review Comment:
This test class redeclares `@Parameterized.Parameter public
SqlTransactionMode sqlTxMode`, but `AbstractBasicIntegrationTransactionalTest`
already defines the same `@Parameterized.Parameter` field. JUnit's
`Parameterized` runner will see both (including inherited public fields), which
can lead to parameter injection errors or ambiguous parameter indices. Remove
the redeclared field and rely on the inherited `sqlTxMode` while keeping the
custom `parameters()` method to restrict modes.
```suggestion
```
--
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]
