Conversation
|
Ignite PR Checker verdict · RunAll build 9344108 · 147 suites ran, 0 reused
Everything below is what it did manage to say.
🔍 1 suite(s) ran fewer tests than on master (tests that never ran can't fail):
❌ 8 blocker(s) in 5 suite(s):
|
c4ff85d to
2527db4
Compare
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds QueryEntity metadata merging across CacheConfiguration calls, including fields, indexes, and type conflicts.
Changes:
- Introduces
QueryEntityMerger. - Updates configuration merging behavior and conflict validation.
- Adds comprehensive tests and adjusts indexing tests.
File summaries
| File | Description |
|---|---|
| modules/indexing/src/test/java/org/apache/ignite/testsuites/IgniteCacheWithIndexingTestSuite.java | Updated as part of this pull request. |
| modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/persistence/IgnitePdsIndexingDefragmentationTest.java | Updated as part of this pull request. |
| modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/DuplicateKeyValueClassesSelfTest.java | Updated as part of this pull request. |
| modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/CacheConfigurationQueryEntityMergeTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/IgnitePdsCorruptedIndexTest.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/internal/processors/query/QueryEntityMerger.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/configuration/CacheConfiguration.java | Updated as part of this pull request. |
Review details
Suppressed comments (3)
modules/core/src/main/java/org/apache/ignite/internal/processors/query/QueryEntityMerger.java:73
- With the default
sqlEscapeAll=false,QueryUtils.normalizeObjectNameuppercases table names before schema creation. Comparing the raw values here makes otherwise compatible fragments usingPersonandPERSONconflict even though they resolve to the same SQL table, so configuration fails before normalization; use the cache's SQL escaping/normalization policy when comparing this property.
res.setTableName(mergeProperty("tableName", ex.getTableName(), in.getTableName()));
modules/core/src/main/java/org/apache/ignite/internal/processors/query/QueryEntityMerger.java:245
- The map is keyed by the effective name from
QueryUtils.indexName, butQueryIndex.equalsalso compares the rawnamefield. Consequently, an unnamed index and an explicitly named index whose name equals the generated name hit the same map entry and are always reported as conflicting, although normalization produces the same index. Compare canonical index definitions rather than the raw object equality here.
if (!existingIdx.equals(incomingIdx))
throw mergeConflict("index[" + idxName + ']', existingIdx, incomingIdx);
modules/core/src/main/java/org/apache/ignite/internal/processors/query/QueryEntityMerger.java:97
- This compares raw key type names, so a supported primitive declaration such as
QueryEntity.setKeyType("int")conflicts withsetIndexedTypes(Integer.class, ...), even thoughsetIndexedTypesboxes primitive classes andQueryUtilsexplicitly resolves/boxes primitive key types. Canonicalize resolved key types before deciding they conflict, while retaining support for non-loadable binary type names.
if (exKeyType != null && inKeyType != null && !Objects.equals(exKeyType, inKeyType)) {
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ); | ||
| } | ||
|
|
||
| QueryEntity res = new QueryEntity(ex); |
There was a problem hiding this comment.
I reproduced the QueryEntityEx metadata loss and investigated the schema-add and cache-configuration merge paths.
The reported problem is real: with the current PR implementation, QueryEntity res = new QueryEntity(ex) can turn a QueryEntityEx into a plain QueryEntity. Which leads to losing sql, preserveKeysOrder, fillAbsentPKsWithDefaults, and the other extended metadata.
However, after tracing the actual lifecycle, it looks like the schema-add case does not require defining merge rules for two different QueryEntityEx instances. The downgrade was caused by a different issue in patchCacheConfiguration().
The reproducer is testSchemaAddPreservesQueryEntityExMetadata(). It uses a statically configured cache with no query entities and then executes a query that creates a table. This DDL operation creates a QueryEntityEx with:
- sql = true
- preserveKeysOrder = true
- fillAbsentPKsWithDefaults = true
The schema-add lifecycle patches the cache configuration twice:
SchemaAddQueryEntityOperation
>>>
GridCacheContext.onSchemaAddQueryEntity() -> patchCacheConfiguration()
>>>
SchemaFinishDiscovery -> DynamicCacheDescriptor.schemaChangeFinish() → patchCacheConfiguration()
Initially I expected this to be a legitimate merge of two extended entities, but it is not. The first and the second patch operate on different CacheConfiguration objects, but CacheConfiguration copy construction keeps the same mutable qryEntities collection. Therefore the first patch modifies the collection that is still visible from the descriptor configuration.
So the "merge" in this scenario is actually an accidentally repeated usage of the same schema-add entity. It is not merging two independently defined QueryEntityEx configurations.
I added an isolated regression test for the underlying issue:
GridCacheUtilsTest.testPatchCacheConfigurationDoesNotModifyOriginalQueryEntities
The test prepares an oldCfg with a materialized empty query-entity collection, calls GridCacheUtils.patchCacheConfiguration(), and verifies that adding the new entity to the patched configuration does not modify oldCfg.
On master / the previous implementation it fails on:
assertTrue(oldCfg.getQueryEntities().isEmpty());because both configurations share the same mutable qryEntities collection.
After making patchCacheConfiguration() use an independent query-entity collection, this test passes.
More importantly, after this change testSchemaAddPreservesQueryEntityExMetadata also passes. The schema-finish patch now sees an empty descriptor configuration and simply installs the incoming QueryEntityEx. It no longer tries to merge two identical entities.
I also checked whether we have a supported runtime path that would require resolving a real conflict between two different QueryEntityEx objects. The relevant cases I checked were:
- The normal
QueryEntityconflicts targeted by this PR are still real and are covered byCacheConfigurationQueryEntityMergeTest. - Adding SQL metadata dynamically to an already indexed cache is rejected, for example:
javax.cache.CacheException: Cache is already indexed: sql-cache
- Joining an active cluster with a static cache configuration that would require schema/configuration merging is rejected:
org.apache.ignite.spi.IgniteSpiException: Failed to join node to the active cluster (the config of the cache 'TEST_CACHE' has to be merged which is impossible on active grid). Deactivate grid and retry node join or clean the joining node.
- I also tested a more specific SQL-vs-API case: create the cache/table through
CREATE TABLEon the existing node and configure the same cache statically on the joining node. This is rejected with:
org.apache.ignite.IgniteCheckedException: Cache configuration mismatch (local cache was created via Ignite API, while remote cache was created via CREATE TABLE): ...
Again, QueryEntityMerger is not reached.
5. It is possible to force QueryEntityEx + QueryEntity manually by obtaining the internal CacheConfiguration through node.context().cache().cacheConfiguration(...) and calling setQueryEntities() directly. In that artificial scenario the current merger can indeed downgrade the entity.
However, this mutates Ignite's internal runtime configuration directly. And as I understand it is not a supported public cache-configuration update path.
I also reviewed the internal setQueryEntities() paths. Normalization replaces the entity collection after clearing it; creation of a new SQL cache installs entities into an initially empty configuration; schema-add is expected to add entities to an empty schema; and the join cases above are validated before conflicting configurations can reach the merger.
Therefore I did not add rules for merging conflicting QueryEntityEx-specific fields. This would introduce new semantics, for example, for two different extended entities we would have to decide what these combinations mean:
- sql: false vs true
- implicitPk: false vs true
- preserveKeysOrder: false vs true
- fillAbsentPKsWithDefaults: false vs true
- primaryKeyInlineSize: 10 vs 20
- affinityKeyInlineSize: 10 vs 20
It is not clear whether they should be conflicts, whether one side should overtake or whether some boolean fields should be combined. I could not find a supported runtime lifecycle that requires making those decisions.
So the fix I propose is:
- keep QueryEntityMerger responsible for actual QueryEntity metadata merging
- make patchCacheConfiguration() independent from the original configuration's mutable query-entity collection
- keep testSchemaAddPreservesQueryEntityExMetadata as the integration regression test
- add GridCacheUtilsTest.testPatchCacheConfigurationDoesNotModifyOriginalQueryEntities
as the focused regression test for the root cause.
With this change, theQueryEntityExcreated by the schema-add operation is processed correctly without introducing merge rules for its extended metadata.
If there is a supported path where two independently created QueryEntityEx instances are expected to be merged, I can add handling for that path as well, but I could not reproduce or identify such a case in the current cache/schema lifecycle.
| .setName(CACHE_NAME); | ||
|
|
||
| grid(0).createCache(ccfg); | ||
| String msg = String.format("Failed to merge query entities due to conflicting metadata " + |
There was a problem hiding this comment.
plz move this message into public string constant (QueryEntityMerger) and use it for comparison here.
| @Test | ||
| public void testIndexingWithIntegerKey() throws Exception { | ||
| test(Function.identity()); | ||
| test(Integer.class, Function.identity()); |
There was a problem hiding this comment.
| test(Integer.class, Function.identity()); | |
| test(indexedKeyType, Function.identity()); |
There was a problem hiding this comment.
test() expects Class<T>, so passing Class<?> will cause type mismatch. I changed test() so that indexedKeyType is used as the key type source directly
|
|
||
| /** Query entities with different value types must not be merged. */ | ||
| @Test | ||
| public void testDifferentValueTypesAreNotMerged() throws Exception { |
There was a problem hiding this comment.
naming confusing, i see test for public API - this test - how it need\plan to work. I mean - one cache can contain numerous of different QueryEntitys, isnt it ? This test need to work perfectly well without your changes i suppose, if i`m right - you need to rename it.
There was a problem hiding this comment.
Correct, we can have entities with the same key type, but with different value type. You’re right, this was working correctly before my changes. This is actually a regression test: I think it’s important to make sure that QueryEntityMerger doesn’t try to merge such entities and doesn’t view them as a conflict. I changed the name and javadoc to make it more obvious.
|
|
||
| /** Query entities with the same value type but different key type are a conflict. */ | ||
| @Test | ||
| public void testConflictingKeyTypesFail() { |
There was a problem hiding this comment.
may be you can remove this test at all in such a case ? : testConflictingKeyTypesForSameValueClass
There was a problem hiding this comment.
This test covers a different scenario and should remain. testQueryEntitiesWithSameKeyTypeAndDifferentValueTypesAreNotMerged() checks that we can have entities with the same key type but different value types. This test checks that 2 entities with the same value type are a conflict, even though they have different key types. Before my changes one of such entities was silently discarded.
| Collection<QueryEntity> entities = node.context().cache() | ||
| .cacheDescriptor(CACHE_NAME) | ||
| .cacheConfiguration() | ||
| .getQueryEntities(); |
There was a problem hiding this comment.
| Collection<QueryEntity> entities = node.context().cache() | |
| .cacheDescriptor(CACHE_NAME) | |
| .cacheConfiguration() | |
| .getQueryEntities(); | |
| Collection<QueryEntity> entities = entities(node); |
|
|
||
| DynamicCacheDescriptor desc = node.context().cache().cacheDescriptor(CACHE_NAME); | ||
|
|
||
| assertTrue(desc.cacheConfiguration().getQueryEntities().isEmpty()); |
There was a problem hiding this comment.
| assertTrue(desc.cacheConfiguration().getQueryEntities().isEmpty()); | |
| assertTrue(entities(node).isEmpty()); |
| if (CACHE_NAME.equals(tbl.cacheName())) | ||
| res = tbl; |
There was a problem hiding this comment.
| if (CACHE_NAME.equals(tbl.cacheName())) | |
| res = tbl; | |
| if (CACHE_NAME.equals(tbl.cacheName())) { | |
| res = tbl; | |
| break; | |
| } |
| SqlTableView tbl = cacheTable(node); | ||
| assertNotNull(tbl); | ||
|
|
||
| assertEquals(tblName, tbl.tableName()); |
There was a problem hiding this comment.
let`s add also ?
assertEquals(1, entities(node).size());
| assertEquals(tblName, tbl.tableName()); | ||
| } | ||
|
|
||
| /** Different configured table names are a conflict. */ |
There was a problem hiding this comment.
| /** Different configured table names are a conflict. */ | |
| /** Different configured table names are lead to conflict. */ |
| /** */ | ||
| private <T> Set<T> mergeSet(Set<T> existing, Set<T> incoming) { | ||
| if (F.isEmpty(existing) && F.isEmpty(incoming)) | ||
| return null; |
There was a problem hiding this comment.
| return null; | |
| return Set.of(); |
| Collection<QueryIndex> incomingIndexes | ||
| ) { | ||
| if (F.isEmpty(existingIndexes) && F.isEmpty(incomingIndexes)) | ||
| return null; |
There was a problem hiding this comment.
| return null; | |
| return List.of(); |
| /** */ | ||
| private QueryEntity findQueryEntity(String valType) { | ||
| if (qryEntities == null) | ||
| return null; |
There was a problem hiding this comment.
I plan to use JSpecify Nullaway frameqork in future, thus we need to be accurate with return types if function is not annotated with @Nullable plz fix it.
| return entity; | ||
| } | ||
|
|
||
| return null; |
| ); | ||
| } | ||
|
|
||
| QueryEntity res = new QueryEntity(ex); |
There was a problem hiding this comment.
probably it`s better to rewrite like :
QueryEntity res = ex instanceof QueryEntityEx ? new QueryEntityEx(ex) : new QueryEntity(ex);
and roll back code in
org.apache.ignite.internal.processors.cache.GridCacheUtils#patchCacheConfiguration
I think it become more clear, wdyt ?
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see tabPR Checkat TC.Bot - Instance 1 or TC.Bot - Instance 2)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.