IGNITE-28352 Calcite. User defined sql function miss entries are written under the same tx lock by zstan · Pull Request #12936 · apache/ignite

zstan · 2026-03-25T11:02:53Z

No description provided.

Copilot

Pull request overview

This PR addresses tx-aware query correctness for Calcite user-defined SQL functions (UDFs) by propagating “modified entries” context across query execution threads so nested queries executed inside UDFs can see the proper transactional changes.

Changes:

Introduces a per-thread “modified entries” holder and wires it through ExecutionServiceImpl/ExecutionContext to reuse tx-change sets when no explicit user transaction is present.
Updates execution-context construction sites (prod + tests) to match the new constructor signature.
Adds a new integration test suite entry and a dedicated integration test covering tx-aware UDF behavior.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/TxAwareModifiedEntriesHolder.java	New ThreadLocal-based holder for tx-modified entries.
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java	Uses the holder to supply tx entries when `userTx == null`; passes holder into local execution context.
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionContext.java	Stores/detaches tx entries in the holder around task execution; accepts holder in ctor.
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExchangeServiceImpl.java	Updates `ExecutionContext` construction to new signature (adds `null`).
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/PlanExecutionTest.java	Updates `ExecutionContext` construction to new signature (adds `null`).
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/AbstractExecutionTest.java	Updates `ExecutionContext` construction to new signature (adds `null`).
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/RuntimeSortedIndexTest.java	Updates `ExecutionContext` construction to new signature (adds `null`).
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementorTest.java	Updates `ExecutionContext` construction to new signature (adds `null`).
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/UserDefinedTxAwareFunctionsIntegrationTest.java	New integration test for tx-aware UDF behavior (including nested queries).
modules/calcite/src/test/java/org/apache/ignite/testsuites/IntegrationTestSuite.java	Registers the new integration test in the suite.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java

...src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionContext.java

Copilot · 2026-03-26T12:42:56Z

...nternal/processors/query/calcite/integration/UserDefinedTxAwareFunctionsIntegrationTest.java

+    /** */
+    @Parameterized.Parameter()
+    public SqlTransactionMode sqlTxMode;


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.

Suggested change

/** */

@Parameterized.Parameter()

public SqlTransactionMode sqlTxMode;

...a/org/apache/ignite/internal/processors/query/calcite/exec/TxAwareModifiedEntriesHolder.java

Copilot · 2026-03-26T12:42:57Z

...main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java

                Commons.parametersMap(msg.parameters()),
-                msg.queryTransactionEntries()
+                msg.queryTransactionEntries(),
+                null


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.

Suggested change

null

modifiedEntriesHolder

…ten under the same tx lock

alex-plekhanov · 2026-04-01T11:05:32Z

...main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java

        RootQuery<Row> qry,
-        MultiStepPlan plan
+        MultiStepPlan plan,
+        TxAwareModifiedEntriesHolder mofiedEntriesHolder


No need to pass this parameter, it's a non static method and always passed as this.modifiedEntriesHolder
Also typo in mofiedEntriesHolder

alex-plekhanov · 2026-04-01T11:19:28Z

...main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java

+     *
+     * @see TransactionConfiguration#isTxAwareQueriesEnabled()
+     */
+    private TxAwareModifiedEntriesHolder modifiedEntriesHolder;


Why not use just static ThreadLocal in ExecutionContext? Looks like we don't need this variable outside of execution context.
When execution context created we can check if qryTxEntries == null and get qryTxEntries from thread local.
It's much simplier (just about 5-10 lines of code)

done, thanks !

alex-plekhanov · 2026-04-01T11:20:11Z

modules/calcite/src/test/java/org/apache/ignite/testsuites/IntegrationTestSuite.java

    MultiDcQueryMappingTest.class,
-    TxWithExceptionalInterceptorTest.class
+    TxWithExceptionalInterceptorTest.class,
+    UserDefinedTxAwareFunctionsIntegrationTest.class


Comma at the end of line please.

alex-plekhanov · 2026-04-01T11:26:30Z

...nternal/processors/query/calcite/integration/UserDefinedTxAwareFunctionsIntegrationTest.java

+    /** Check tx aware udf execution results. */
+    @Test
+    public void testTxAwareUserDefinedFunc() {
+        assertTrue(nodeCount() > 1);


Override nodeCount() instead?

it just a defence if someone try to change it in future (for example - startup speedup with only 1 node)

alex-plekhanov · 2026-04-01T11:27:21Z

...nternal/processors/query/calcite/integration/UserDefinedTxAwareFunctionsIntegrationTest.java

+    }
+
+    /** */
+    private static class City {


Let's reuse Employer entity from superclass.

alex-plekhanov · 2026-04-01T11:46:44Z

...nternal/processors/query/calcite/integration/UserDefinedTxAwareFunctionsIntegrationTest.java

+
+        /** */
+        @QuerySqlFunction
+        public static List<List<?>> name(int id) {


nameTableFunc?

alex-plekhanov · 2026-04-01T11:48:07Z

...nternal/processors/query/calcite/integration/UserDefinedTxAwareFunctionsIntegrationTest.java

+
+        /** */
+        @QuerySqlFunction
+        public List<List<?>> customTableFuncInner() {


Redundant, can be used customTableFunc

alex-plekhanov · 2026-04-01T11:49:44Z

...nternal/processors/query/calcite/integration/UserDefinedTxAwareFunctionsIntegrationTest.java

+
+        IgniteCache<Integer, Object> cache = client.cache(DEFAULT_CACHE_NAME);
+
+        /*The pool size should be greater than the maximum number of concurrent queries initiated by UDFs*/


Spaces before/after comment and point at the end.

alex-plekhanov · 2026-04-01T11:50:34Z

...nternal/processors/query/calcite/integration/UserDefinedTxAwareFunctionsIntegrationTest.java

+        client.getOrCreateCache(cacheConfig());
+
+        IgniteCache<Integer, Object> cache = client.cache(DEFAULT_CACHE_NAME);


IgniteCache<Integer, Object> cache = client.getOrCreateCache(cacheConfig());

alex-plekhanov · 2026-04-01T11:51:01Z

...nternal/processors/query/calcite/integration/UserDefinedTxAwareFunctionsIntegrationTest.java

+        client.getOrCreateCache(cacheConfig());
+
+        List<List<Object>> refResults = new ArrayList<>();
+
+        IgniteCache<Integer, Object> cache = client.cache(DEFAULT_CACHE_NAME);


IgniteCache<Integer, Object> cache = client.getOrCreateCache(cacheConfig());

sonarqubecloud · 2026-04-07T13:02:37Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

zstan changed the title ~~IGNITE-28352 Calcite. User defined sql fumction miss entries are written under the same tx lock~~ IGNITE-28352 Calcite. User defined sql function miss entries are written under the same tx lock Mar 26, 2026

zstan requested review from alex-plekhanov and Copilot March 26, 2026 12:36

Copilot started reviewing on behalf of zstan March 26, 2026 12:37 View session

zstan force-pushed the ignite-28352 branch from b257b0c to c0194c9 Compare March 26, 2026 12:40

Copilot AI reviewed Mar 26, 2026

View reviewed changes

zstan added 2 commits March 30, 2026 13:42

IGNITE-28352 Calcite. User defined sql function miss entries are writ…

04612da

…ten under the same tx lock

typo fix

a150f74

zstan force-pushed the ignite-28352 branch from 199caaa to a150f74 Compare March 30, 2026 10:42

zstan added 2 commits March 31, 2026 08:51

simplify

7cbf6cc

query pool size

913b755

alex-plekhanov reviewed Apr 1, 2026

View reviewed changes

zstan added 2 commits April 2, 2026 12:39

fix after review

c4ca2dc

code style

2409174

zstan requested a review from alex-plekhanov April 3, 2026 09:17

zstan added 2 commits April 5, 2026 18:53

revert pool size

2f49bfc

change thread num size

8d1e558

alex-plekhanov approved these changes Apr 7, 2026

View reviewed changes

zstan merged commit 15f2122 into apache:master Apr 8, 2026
10 checks passed

	/** */
	@Parameterized.Parameter()
	public SqlTransactionMode sqlTxMode;


		IgniteCache<Integer, Object> cache = client.cache(DEFAULT_CACHE_NAME);

		/The pool size should be greater than the maximum number of concurrent queries initiated by UDFs/

		client.getOrCreateCache(cacheConfig());

		IgniteCache<Integer, Object> cache = client.cache(DEFAULT_CACHE_NAME);

Conversation

zstan commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 7, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants