oleg-vlsk commented on code in PR #13583: URL: https://github.com/apache/ignite/pull/13583#discussion_r4056487116
########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/QueryEntityMerger.java: ########## @@ -0,0 +1,261 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.processors.query; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import javax.cache.CacheException; +import org.apache.ignite.cache.QueryEntity; +import org.apache.ignite.cache.QueryIndex; +import org.apache.ignite.internal.util.typedef.F; + +/** Utility for merging compatible {@link QueryEntity} metadata. */ +public final class QueryEntityMerger { + /** */ + private final String cacheName; + + /** */ + private QueryEntityMerger(String cacheName) { + this.cacheName = cacheName; + } + + /** + * Merges incoming query entity metadata into existing entity. + * + * @param cacheName Cache name. + * @param existing Existing query entity. + * @param incoming Incoming query entity. + * @return Merged query entity. + * @throws CacheException If entities contain conflicting metadata. + */ + public static QueryEntity merge(String cacheName, QueryEntity existing, QueryEntity incoming) { + return new QueryEntityMerger(cacheName).merge0(existing, incoming); + } + + /** */ + private QueryEntity merge0(QueryEntity ex, QueryEntity in) { + if (!Objects.equals(ex.findValueType(), in.findValueType())) { + throw new CacheException( + "Failed to merge query entities because value types differ " + + "[cacheName=" + cacheName + + ", existingValueType=" + ex.findValueType() + + ", incomingValueType=" + in.findValueType() + ']' + ); + } + + QueryEntity res = new QueryEntity(ex); Review 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: ```text 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: ```text 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: ```java 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: 1. The normal `QueryEntity` conflicts targeted by this PR are still real and are covered by `CacheConfigurationQueryEntityMergeTest`. 2. Adding SQL metadata dynamically to an already indexed cache is rejected, for example: ```text javax.cache.CacheException: Cache is already indexed: sql-cache ``` 3. Joining an active cluster with a static cache configuration that would require schema/configuration merging is rejected: ```text 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. ``` 4. I also tested a more specific SQL-vs-API case: create the cache/table through `CREATE TABLE` on the existing node and configure the same cache statically on the joining node. This is rejected with: ```text 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, the `QueryEntityEx` created 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
