IGNITE-28277 CacheInterceptor need to take into account cache keepBinary mode by zstan · Pull Request #12911 · apache/ignite

zstan · 2026-03-18T12:13:47Z

I make a fix only for calcite related part, honestly - i afraid to make an equal changes on h2 related part and prefer to store it - as is. If approach is ok - i fill follow up issue for related documentation change.

Copilot

Pull request overview

This PR extends Calcite integration coverage around CacheInterceptor#onBeforePut behavior (particularly keep-binary vs unwrapped values) and adjusts Calcite DML execution to respect query keep-binary settings, while also applying several small cleanup/typo fixes across tests and core code.

Changes:

Add a new Calcite integration test covering interceptor value wrapping/unwrapping with/without keep-binary mode and include it in the Calcite integration suite.
Update Calcite ModifyNode to choose keepBinary() based on QueryProperties.keepBinary() and propagate session/application attributes to DML cache operations.
Introduce IgniteInternalCache#withApplicationAttributes(...) and wire it through cache proxies/contexts; plus assorted small cleanups (double semicolons, typo fixes, unused constant removal).

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
modules/indexing/src/test/java/org/apache/ignite/internal/processors/query/stat/PSUStatisticsTypesTest.java	Removes an extra semicolon in test SQL string construction.
modules/indexing/src/test/java/org/apache/ignite/internal/processors/query/SqlSystemViewsSelfTest.java	Removes an extra semicolon in test setup.
modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/SqlFieldsQuerySelfTest.java	Removes an unused `INSERT` constant.
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/standbycluster/IgniteStandByClusterTest.java	Fixes typo in nested plugin provider name (`StanBy` → `StandBy`).
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/binary/CacheKeepBinaryWithInterceptorTest.java	Renames test interceptor classes for clarity; makes a test field `final` and suppresses unused warning.
modules/core/src/test/java/META-INF/services/org.apache.ignite.plugin.PluginProvider	Updates service registration entry to match renamed provider class.
modules/core/src/main/java/org/apache/ignite/internal/util/BasicRateLimiter.java	Removes an extra semicolon in rate limiter resync logic.
modules/core/src/main/java/org/apache/ignite/internal/processors/job/GridJobWorker.java	Removes an extra semicolon in logging statement.
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/continuous/CacheContinuousQueryPartitionRecovery.java	Removes an extra semicolon in boolean expression.
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/atomic/GridDhtAtomicCache.java	Removes an extra semicolon after lock acquisition.
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteInternalCache.java	Adds `withApplicationAttributes(Map<String,String>)` projection API.
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java	Implements `withApplicationAttributes` on the internal cache proxy (but see review comments).
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheAdapter.java	Implements `withApplicationAttributes` for adapter-based projections (but see review comments).
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheOperationContext.java	Adds `withApplicationAttributes(...)` to propagate app/session attributes through operation context.
modules/core/src/main/java/org/apache/ignite/internal/managers/discovery/GridDiscoveryManager.java	Removes an extra semicolon in an assertion.
modules/calcite/src/test/java/org/apache/ignite/testsuites/IntegrationTestSuite.java	Registers the new Calcite interceptor integration test in the suite.
modules/calcite/src/test/java/org/apache/ignite/internal/processors/tx/TxWithExceptionalInterceptorTest.java	Ensures grids are stopped in `afterTest()` to avoid leakage across parameter runs.
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/CacheWithInterceptorIntegrationTest.java	New integration test validating interceptor `onBeforePut` value wrapping/unwrapping with keep-binary mode.
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/CancelTest.java	Removes an extra semicolon in query entity builder.
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/ModifyNode.java	Uses `QueryProperties.keepBinary()` to decide whether to apply `keepBinary()` for DML; propagates session attributes via `withApplicationAttributes`.
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/HashJoinNode.java	Removes an extra semicolon in downstream push.

Comments suppressed due to low confidence (1)

modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java:321

In withApplicationAttributes, the opCtx == null branch constructs a CacheOperationContext with skipReadThrough set to true (see the second boolean argument). That changes cache behavior (read-through) just by setting application attributes, which is very likely unintended and inconsistent with GridCacheAdapter.withApplicationAttributes (which keeps it false). Please preserve the existing flags (i.e., do not enable skipReadThrough here).

    @Override public GridCacheProxyImpl<K, V> withApplicationAttributes(Map<String, String> attrs) {
        CacheOperationContext prev = gate.enter(opCtx);

        try {
            return new GridCacheProxyImpl<>(ctx, delegate,
                opCtx != null ? opCtx.setApplicationAttributes(attrs) :
                    new CacheOperationContext(
                        false,
                        true,
                        false,
                        null,
                        false,
                        null,
                        false,
                        null,
                        attrs));

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

Copilot

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

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

Copilot

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java:321

When opCtx is null, withApplicationAttributes stores the caller-provided attrs map directly in the new CacheOperationContext. This differs from the opCtx != null path (which copies via setApplicationAttributes) and can leak later mutations or preserve a non-serializable map implementation. Consider always storing a defensive copy (and optionally rejecting null like the public IgniteCache#withApplicationAttributes implementations do).

    @Override public GridCacheProxyImpl<K, V> withApplicationAttributes(Map<String, String> attrs) {
        CacheOperationContext prev = gate.enter(opCtx);

        try {
            return new GridCacheProxyImpl<>(ctx, delegate,
                opCtx != null ? opCtx.setApplicationAttributes(attrs) :
                    new CacheOperationContext(
                        false,
                        false,
                        false,
                        null,
                        false,
                        null,
                        false,
                        null,
                        attrs));

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

alex-plekhanov · 2026-04-01T09:16:32Z

+    @Override protected void afterTest() throws Exception {
+        super.afterTest();
+
+        stopAllGrids(true);


Do we still need stopAllGrids at beforeTest?

alex-plekhanov · 2026-04-01T09:20:12Z

+            if (keepBinary)
+                assertTrue(newVal instanceof BinaryObject);
+            else
+                assertFalse(newVal instanceof BinaryObject);


assertEquals(keepBinary, newVal instanceof BinaryObject);

alex-plekhanov · 2026-04-01T09:21:05Z

+        var pureCacheCfg = new CacheConfiguration<Integer, Object>("Pure")
+            .setAtomicityMode(TRANSACTIONAL)
+            .setSqlSchema("PUBLIC")
+            .setInterceptor(new TestAlwaysUnwrappedValCacheInterceptor())


new TestCacheInterceptor(false) and remove redundant class?

done, thanks !

alex-plekhanov · 2026-04-01T09:24:06Z

+        var entity1 = new QueryEntity()
+            .setTableName("CITY")
+            .setKeyType(Integer.class.getName())
+            .setValueType(City.class.getName())
+            .addQueryField("id", Integer.class.getName(), null)
+            .addQueryField("name", String.class.getName(), null)
+            .setKeyFieldName("id");
+
+        var entity2 = new QueryEntity()
+            .setTableName("PERSON")
+            .setKeyType(Integer.class.getName())
+            .setValueType(Person.class.getName())
+            .addQueryField("id", Integer.class.getName(), null)
+            .addQueryField("name", String.class.getName(), null)
+            .addQueryField("city_id", Integer.class.getName(), null)
+            .setKeyFieldName("id");


Why don't use @SqlQueryField annotation and simplify configuration?

done, but new approach looks like not more simply ) as i see

zstan · 2026-04-02T06:37:22Z

append jmh test, results:

Benchmark                             (engine)  (keepBinary)   Mode  Cnt     Score     Error  Units
JmhCacheWithInterceptorBenchmark.put   CALCITE          true  thrpt   10  3504.528 ± 142.769  ops/s
JmhCacheWithInterceptorBenchmark.put   CALCITE         false  thrpt   10  3461.187 ± 165.752  ops/s

zstan · 2026-04-02T07:29:54Z

update op:

Benchmark                                (engine)  (keepBinary)   Mode  Cnt     Score     Error  Units
JmhCacheWithInterceptorBenchmark.update   CALCITE          true  thrpt   10  2672.824 ± 119.782  ops/s
JmhCacheWithInterceptorBenchmark.update   CALCITE         false  thrpt   10  2702.268 ± 171.562  ops/s

alex-plekhanov · 2026-04-02T06:53:08Z

    MultiDcQueryMappingTest.class,
-    TxWithExceptionalInterceptorTest.class
+    TxWithExceptionalInterceptorTest.class,
+    CacheWithInterceptorIntegrationTest.class


Please add comma at the end of line

alex-plekhanov · 2026-04-02T14:08:29Z

+
+        /** */
+        @QuerySqlField
+        int city_id;


Let's use java style field naming (cityId) and add alias

alex-plekhanov · 2026-04-02T14:09:13Z

+        int incParam = 0;
+
+        try (Transaction tx = client.transactions().txStart(PESSIMISTIC, READ_COMMITTED)) {
+            client.context().query().querySqlFields(new SqlFieldsQuery("INSERT INTO PUBLIC.PURE(id, name) VALUES (?, 'val')")


Let's test not only INSERT, but also UPDATE and DELETE (with onBeforeRemove in intercepter)

alex-plekhanov · 2026-04-02T14:12:28Z

+            .addQueryField("name", String.class.getName(), null)
+            .setKeyFieldName("id")
+            .setValueFieldName("name");
+


Let's also add entity:

var entity1 = new QueryEntity() .setTableName("Complex") .setKeyType(Integer.class.getName()) .setValueType("ComplexKey") .addQueryField("id", Integer.class.getName(), null) .addQueryField("name", String.class.getName(), null) .addQueryField("val", Integer.class.getName(), null) .setKeyFieldName("id");

And cache

var complexCacheCfg = new CacheConfiguration<Integer, Object>("complex") .setAtomicityMode(TRANSACTIONAL) .setSqlSchema("PUBLIC") .setQueryEntities(List.of(entity1));

Without iterceptor.
DML on this cache will fail on interceptor, since there is no such class for deserialization, but without interceptor it should pass until someone add getValue for entry processor in ModifyNode.

done, but i still miss an idea - why we need it ?)

To ensure that nobody will break this case in the future by adding entry.getValue() to any entry processor in ModifyNode

alex-plekhanov · 2026-04-02T14:18:35Z

+                .setArgs(incParam++), keepBinary).getAll();
+            client.context().query().querySqlFields(new SqlFieldsQuery("INSERT INTO PUBLIC.CITY(id, name) VALUES (?, 'val')")
+                .setArgs(incParam++), keepBinary).getAll();
+            client.context().query().querySqlFields(new SqlFieldsQuery("INSERT INTO PUBLIC.PERSON(id, name, city_id) VALUES (?, 'val', 1)")


client.context().query().querySqlFields - it's a private API, maybe it's better to check with public API (via cache.withKeepBinary)

zstan · 2026-04-03T08:04:50Z

Benchmark                                (engine)  (keepBinary)   Mode  Cnt     Score     Error  Units
JmhCacheWithInterceptorBenchmark.update   CALCITE          true  thrpt   10  4372.599 ± 157.473  ops/s
JmhCacheWithInterceptorBenchmark.update   CALCITE         false  thrpt   10  4429.750 ± 140.710  ops/s

alex-plekhanov · 2026-04-03T13:09:13Z

+        }
+
+        /** {@inheritDoc} */
+        @Override public @Nullable Object onBeforePut(Cache.Entry<Integer, Object> entry, Object newVal) {


What about onBeforeRemove?

github-actions · 2026-04-05T11:05:28Z

Possible compatibility issues. Please, check rolling upgrade cases

This PR modifies protected classes (with Order annotation).
Changes to these classes can break rolling upgrade compatibility.

Affected files:

modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/message/QueryStartRequest.java

sonarqubecloud · 2026-04-05T11:12:59Z

Quality Gate failed

Failed conditions
3 New Code Smells (required ≤ 1)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

zstan force-pushed the ignite-28277 branch 2 times, most recently from 8be5c14 to 84d439a Compare March 30, 2026 10:44

zstan requested a review from Copilot March 30, 2026 10:45

Copilot started reviewing on behalf of zstan March 30, 2026 10:46 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

zstan force-pushed the ignite-28277 branch from 84d439a to 50f1461 Compare March 30, 2026 11:03

zstan requested a review from Copilot March 30, 2026 11:03

Copilot started reviewing on behalf of zstan March 30, 2026 11:04 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

Comment thread modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheAdapter.java

Comment thread modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java

IGNITE-28277 Append additional tests for CacheInterceptor#onBeforePut

2d42681

zstan force-pushed the ignite-28277 branch from 50f1461 to 2d42681 Compare March 30, 2026 11:16

zstan requested a review from Copilot March 30, 2026 11:17

Copilot started reviewing on behalf of zstan March 30, 2026 11:18 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

zstan requested a review from alex-plekhanov March 30, 2026 13:49

zstan changed the title ~~IGNITE-28277 Append additional tests for CacheInterceptor#onBeforePut~~ IGNITE-28277 CacheInterceptor need to take into account cache keepBinary mode Apr 1, 2026

alex-plekhanov reviewed Apr 1, 2026

View reviewed changes

zstan added 2 commits April 1, 2026 17:52

fix after review

df3cce8

add jmh

4f9d4c9

new jmh

92003a3

alex-plekhanov reviewed Apr 2, 2026

View reviewed changes

fix after review

c88032b

github-actions Bot added the compatibility label Apr 3, 2026

zstan added 2 commits April 3, 2026 12:03

code style

2c8e274

complex cache

0e7733a

alex-plekhanov reviewed Apr 3, 2026

View reviewed changes

add onBeforeRemove

8f7319e

alex-plekhanov approved these changes Apr 6, 2026

View reviewed changes

zstan merged commit e8f16e0 into apache:master Apr 6, 2026
8 of 10 checks passed

Conversation

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

Uh oh!

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 left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

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!

zstan Apr 1, 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!

Uh oh!

zstan commented Apr 2, 2026

Uh oh!

zstan commented Apr 2, 2026

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!

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!

zstan commented Apr 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 5, 2026

Possible compatibility issues. Please, check rolling upgrade cases

Uh oh!

sonarqubecloud Bot commented Apr 5, 2026

Quality Gate failed

zstan commented Mar 18, 2026 •

edited

Loading

zstan Apr 1, 2026 •

edited

Loading