IGNITE-28724 Calcite engine. Support SELECT ... FOR UPDATE query by vldpyatkov · Pull Request #13366 · apache/ignite

vldpyatkov · 2026-07-13T22:12:21Z

https://issues.apache.org/jira/browse/IGNITE-28724

…tion (no-op)

zstan · 2026-08-04T08:50:38Z

+    List<SqlNode> ofCols = null;
+    SqlIdentifier col;
+    Long waitSeconds = null;
+    String waitValue;
+}
+{
+    qry = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY) { s = span(); }
+    [
+        LOOKAHEAD(<FOR> <UPDATE>)
+        <FOR> <UPDATE>
+        [
+            LOOKAHEAD(<OF>)
+            <OF>
+            {
+                ofCols = new ArrayList<SqlNode>();
+            }
+            col = CompoundIdentifier() { ofCols.add(col); }
+            (
+                <COMMA> col = CompoundIdentifier() { ofCols.add(col); }
+            )*
+            { ofList = new SqlNodeList(ofCols, s.pos()); }
+        ]
+        [
+            LOOKAHEAD(<WAIT>)
+            <WAIT> <UNSIGNED_INTEGER_LITERAL>
+            {
+                waitValue = token.image;
+
+                try {
+                    waitSeconds = Long.parseLong(waitValue);
+                }


Suggested change

List<SqlNode> ofCols = null;

SqlIdentifier col;

Long waitSeconds = null;

String waitValue;

}

{

qry = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY) { s = span(); }

[

LOOKAHEAD(<FOR> <UPDATE>)

<FOR> <UPDATE>

[

LOOKAHEAD(<OF>)

<OF>

{

ofCols = new ArrayList<SqlNode>();

}

col = CompoundIdentifier() { ofCols.add(col); }

(

<COMMA> col = CompoundIdentifier() { ofCols.add(col); }

)*

{ ofList = new SqlNodeList(ofCols, s.pos()); }

]

[

LOOKAHEAD(<WAIT>)

<WAIT> <UNSIGNED_INTEGER_LITERAL>

{

waitValue = token.image;

try {

waitSeconds = Long.parseLong(waitValue);

}

SqlIdentifier col;

Long waitSeconds = null;

}

{

qry = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY) { s = span(); }

[

LOOKAHEAD(<FOR> <UPDATE>)

<FOR> <UPDATE>

[

LOOKAHEAD(<OF>)

<OF>

{

List<SqlNode> columns = new ArrayList<SqlNode>();

}

col = CompoundIdentifier() { columns.add(col); }

(

<COMMA> col = CompoundIdentifier() { columns.add(col); }

)*

{ ofList = new SqlNodeList(columns, s.pos()); }

]

[

LOOKAHEAD(<WAIT>)

<WAIT> <UNSIGNED_INTEGER_LITERAL>

{

String waitValue = token.image;

try {

waitSeconds = Long.parseLong(waitValue);

}

The previous variant is fit for existing code, but I can rewrite this close as you wish)

I thought again.
I guess I won't be able to move the variable declarations inside of the Java code here. Because it is a common approach in this file, and I don't want to be the first who violates it.

zstan · 2026-08-04T08:51:22Z

+{
+    final Span s;
+    SqlNode qry;
+    SqlNodeList ofList = null;


rename it plz

The same, but I see you want to put this in order. Ok, I will try.

still the same ?

Fixed variable names and made the Java code slightly simpler.

zstan · 2026-08-04T09:53:35Z

+                IgniteResource.INSTANCE.selectForUpdateRequiresPessimisticTx().str(),
+                IgniteQueryErrorCode.UNSUPPORTED_OPERATION);
+
+        // waitSeconds: null = use tx remaining time (0), 0 = NOWAIT (-1), positive = ms.


(0) and (-1) in braces are confused, what is the purpose of such a comment ? plz extend it or remove

I translate it right now. If you get in waitSeconds after parsing the query string, null it, which means you have to pass wait milliseconds 0, which means wait until the transaction is alive. If you get 0 (in waitSeconds) you have to pass -1L (or any negative digit) with means nowait, or if you get a digit, you have to convert seconds to milliseconds and pass the result.
I will extend the comment here.

zstan · 2026-08-04T09:57:39Z

+            waitMs = waitSeconds * 1000L;
+
+        // Zero means that retries are limited only by the transaction or query timeout.
+        long deadline = waitMs > 0


Suggested change

long deadline = waitMs > 0

long attemptsTimeout = waitMs > 0

I do not think there is a good change because the deadline is not a timeout; it is a time after which the query execution has to be finished.

zstan · 2026-08-04T10:16:26Z

+
+        if (userTx == null || !userTx.pessimistic())
+            throw new IgniteSQLException(
+                IgniteResource.INSTANCE.selectForUpdateRequiresPessimisticTx().str(),


seems we need to align this 'strange' as for me message with :

org.apache.ignite.internal.processors.cache.GridCacheAdapter#lockTxEntriesAsync -> new IgniteCheckedException("Failed to acquire transactional lock in optimistic transaction."));

or ignore this check here and it will be raized a bit later, need to make sure that SqlException will be raized.

The message is not only for optomistic transaction is used. It will be appear when anyone try to liunch SELECT FOR UPDATE without transaction at all. For this reason is has a general statement.

zstan · 2026-08-04T11:37:28Z

+
+            IgniteTable igniteTable = (IgniteTable)schemaPlus.getTable(target.tableName());
+
+            if (igniteTable == null)


is it sys view case ? if so - it need to be handled more informative i suppose

I added a dedicated test for the system table. It fills in the other code fragment.
This exception is rather for completeness than for the specific case. I think we can be there in case where table removed concurrently.

zstan · 2026-08-04T11:59:07Z

+        IgniteInternalFuture<?> lockFut = GridTestUtils.runAsync(() -> {
+            try (Transaction tx = ignite0.transactions().txStart(PESSIMISTIC, READ_COMMITTED)) {
+                assertRows(
+                    sql("SELECT p.id FROM Person p JOIN Dept d ON p.deptId = d.id WHERE p.id = 1 FOR UPDATE"),


why you avoid to inherit from AbstractBasicIntegrationTest ? and use all infrastructure like :

assertQuery("SELECT p.id FROM Person p JOIN Dept d ON p.deptId = d.id WHERE p.id = 1 FOR UPDATE") .returns(1) .check();

These tests do not read the transaction in SQL.
So I can try, but it leads to fixing the base class.

zstan · 2026-08-04T12:00:54Z

+/**
+ * Integration tests for {@code SELECT ... FOR UPDATE} syntax.
+ */
+public class SelectForUpdateIntegrationTest extends GridCommonAbstractTest {


sys view tests are absent

virtual tables are absent table(system_range(1, 4)

non PK involved tests are absent, or i miss ?

Added a test

Added a test

There are a lot of tests without condition on PK.

tkalkirill · 2026-08-04T14:06:48Z

+     *
+     * @return Result cursor when locking succeeds, or {@code null} when the SELECT must be executed again.
+     */
+    @Nullable private FieldsQueryCursor<List<?>> tryExecuteForUpdate(


The method is a bit hard to read; could you split the different parts into separate methods?

I split the method into three parts:

Collect lock batches,

Locks acquiring,

Prepare result.

tkalkirill · 2026-08-04T14:20:02Z

    }

+    /** */
+    private void validateVersionColumnDmlTarget(SqlIdentifier id) {


I don't think other system fields should be modifiable during an update either. Could you check that? If so, we need to fix it for all of them preferably in a separate ticket, ideally one addressed before this one, to avoid bloating the changes here.

For the other system fields, the circumstances are more complicated.
For example, field _val can be used if it is the only field for a simple type like String or Integer.
A _key field is available to use in the INSERT command.

tkalkirill · 2026-08-04T14:35:34Z

    @Override public boolean isUpdateAllowed(RelOptTable tbl, int colIdx) {
        final CacheColumnDescriptor desc = descriptors[colIdx];

+        if (QueryUtils.VER_FIELD_NAME.equals(desc.name()))


Maybe we could introduce a boolean field or something similar to avoid checking strings?

I fied it as desc instanceof SystemDescriptor, it not clear because we call _key and _val also system fileds, but they are instances of the other class (KeyValDescriptor).
Anyway is is correct right now.

tkalkirill · 2026-08-04T14:37:32Z

            // Both WHEN MATCHED and WHEN NOT MATCHED clauses in MERGE.
-            assert rowColumnsCnt == descriptors.length * 2 + updateColList.size() : "Unexpected columns count: " +
-                rowColumnsCnt;
+            assert rowColumnsCnt == 2 * descriptors.length + updateColList.size() : "Unexpected columns count: "


It looks like you haven't changed anything here other than repositioning the multiplication operator.

Yes, I changed it many times and finally returned to the origin.

tkalkirill · 2026-08-05T09:00:05Z

    protected List<List<?>> sql(IgniteEx ignite, String sql, Object... params) {
-        // {@code sql} can contain more than one query.
-        List<FieldsQueryCursor<List<?>>> allCurs = queryProcessor(ignite).query(queryContext(), "PUBLIC", sql, params);
+        Transaction tx = ignite.transactions().tx();


I suggest not keeping these changes in this class but moving them to the appropriate ones, or creating a subclass of AbstractBasicIntegrationTest and defining this logic there.

I like this change. Moreover, I think finally we understand that the AbstractBasicIntegrationTest.java is not the best base class.
Probably, we will make a refactoring in the future in order to it uses a public APY.

tkalkirill · 2026-08-05T09:07:03Z

+
+    /** */
+    @Test
+    public void testSystemColumnsAreHiddenFromSelectStar() throws Exception {


Column name validation is missing.

This check is based on a column count.
The person table has fields id, name, and age, so this assertEquals(3, rows.get(0).size()) passes.

vldpyatkov added 8 commits July 13, 2026 12:37

IGNITE-28864 Add technical columns to Calcite engin SQL scan

85701ce

IGNITE-28868 Calcite SQL: Add SELECT ... FOR UPDATE syntax and valida…

805e26e

…tion (no-op)

IGNITE-28724 Calcite engine. Support SELECT ... FOR UPDATE query

1f6eb89

Added ability to execute SELECT FOR UPDATE with JOIN cluse.

e997c52

SELECT FOR UPDATE retry until timeout.

59e3dfd

Removed _scr technical column.

c7360c9

Self-reviewed.

1c38a8b

Code style.

e9db1fe

zstan reviewed Jul 14, 2026

View reviewed changes

Comment thread .../core/src/main/java/org/apache/ignite/internal/processors/query/QueryTypeDescriptorImpl.java

zstan reviewed Aug 4, 2026

View reviewed changes

vldpyatkov added 2 commits August 4, 2026 21:08

Fixed after review for E.Stanilovskiy.

d900a7d

Merge branch 'master' into igite-28724

1299046

tkalkirill reviewed Aug 5, 2026

View reviewed changes


		IgniteTable igniteTable = (IgniteTable)schemaPlus.getTable(target.tableName());

		if (igniteTable == null)

Uh oh!

Conversation

vldpyatkov commented Jul 13, 2026

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zstan Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

zstan Aug 4, 2026 •

edited

Loading