Skip to content

IGNITE-28907 Support merging QueryEntity metadata in CacheConfiguration - #13583

Open
oleg-vlsk wants to merge 5 commits into
apache:masterfrom
oleg-vlsk:IGNITE-28907
Open

oleg-vlsk wants to merge 5 commits into
apache:masterfrom
oleg-vlsk:IGNITE-28907

Conversation

@oleg-vlsk

Copy link
Copy Markdown
Contributor

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

  • There is a single JIRA ticket related to the pull request.
  • The web-link to the pull request is attached to the JIRA ticket.
  • The JIRA ticket has the Patch Available state.
  • The pull request body describes changes that have been made.
    The description explains WHAT and WHY was made instead of HOW.
  • The pull request title is treated as the final commit message.
    The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
  • A reviewer has been mentioned through the JIRA comments
    (see the Maintainers list)
  • The pull request has been checked by the Teamcity Bot and
    the green visa attached to the JIRA ticket (see tab PR Check at 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.

@oleg-vlsk

Copy link
Copy Markdown
Contributor Author

Ignite PR Checker verdict · RunAll build 9344108 · 147 suites ran, 0 reused

⚠️ This run doesn't cover the PR fully:

  • 2 suite(s) have no reliable result (compilation error, timeout, crash)
  • 1 suite(s) ran far fewer tests than the same suites on master
  • a newer run is still going — its unfinished suites can still fail

Everything below is what it did manage to say.

⚠️ 2 broken suite(s) (no reliable run):

  • Platform C++ CMake (Linux Clang): IgniteOdbcTest terminated abnormally - the test binary died, its remaining tests never ran · Number of tests 595 is 43% less than 1047 in build #43233 — ran 595 of master's 1047 tests
  • Platform C++ CMake (Win x64 | Release): non-zero exit code · Number of tests 465 is 56% less than 1047 in build #19133 — ran 465 of master's 1047 tests

🔍 1 suite(s) ran fewer tests than on master (tests that never ran can't fail):

  • Platform C++ CMake (Linux): 595 tests vs 1047 on master (−43%)

8 blocker(s) in 5 suite(s):

  • ZooKeeper (Discovery) 1: org.apache.ignite.spi.discovery.zk.ZookeeperDiscoverySpiTestSuite1: org.apache.ignite.spi.discovery.zk.internal.ZookeeperDiscoverySplitBrainTest.testSimpleSplitBrain
  • ZooKeeper (Discovery) 1: org.apache.ignite.spi.discovery.zk.ZookeeperDiscoverySpiTestSuite1: org.apache.ignite.spi.discovery.zk.internal.ZookeeperDiscoverySplitBrainTest.testAlmostSplitBrain
  • ZooKeeper (Discovery) 1: org.apache.ignite.spi.discovery.zk.ZookeeperDiscoverySpiTestSuite1: org.apache.ignite.spi.discovery.zk.internal.ZookeeperDiscoverySplitBrainTest.testNotActualSplitBrain
  • Platform .NET (Core Linux): Apache.Ignite.Core.Tests.DotNetCore: Apache.Ignite.Core.Tests.Client.Services.ServicesAwarenessTests.TestClusterTopologyChanges("PlatformTestService")
  • Index Query API: org.apache.ignite.cache.query.IndexQueryTestSuite: org.apache.ignite.cache.query.IndexQueryRangeTest.testRangeQueries[qryPar=1 atomicity=TRANSACTIONAL mode=REPLICATED node=CRD backups=0 idxName=PERSON_ID_IDX duplicates=1]
  • Queries 6: org.apache.ignite.testsuites.IgniteBinaryCacheQueryTestSuite6: org.apache.ignite.internal.processors.cache.index.DuplicateKeyValueClassesSelfTest.testDuplicateValueClass
  • PDS (Indexing): org.apache.ignite.testsuites.IgnitePdsWithIndexingCoreTestSuite: org.apache.ignite.internal.processors.cache.persistence.db.wal.IgniteWalRebalanceTest.testWithLocalWalChange
  • PDS (Indexing): org.apache.ignite.testsuites.IgnitePdsWithIndexingCoreTestSuite: org.apache.ignite.internal.processors.cache.persistence.IgnitePdsCorruptedIndexTest.testCorruption

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.normalizeObjectName uppercases table names before schema creation. Comparing the raw values here makes otherwise compatible fragments using Person and PERSON conflict 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, but QueryIndex.equals also compares the raw name field. 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 with setIndexedTypes(Integer.class, ...), even though setIndexedTypes boxes primitive classes and QueryUtils explicitly 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);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants