anton-vinogradov commented on code in PR #13236: URL: https://github.com/apache/ignite/pull/13236#discussion_r3520974879
########## modules/core/src/test/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessageSerializationTest.java: ########## @@ -0,0 +1,193 @@ +/* + * 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.schema.message; + +import java.io.Externalizable; +import java.io.Serializable; +import java.lang.reflect.Modifier; +import java.math.BigDecimal; +import java.nio.ByteBuffer; +import java.util.Arrays; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.apache.ignite.cache.QueryEntity; +import org.apache.ignite.cache.QueryIndex; +import org.apache.ignite.cache.QueryIndexType; +import org.apache.ignite.internal.CoreMessagesProvider; +import org.apache.ignite.internal.direct.DirectMessageReader; +import org.apache.ignite.internal.direct.DirectMessageWriter; +import org.apache.ignite.internal.managers.communication.IgniteMessageFactoryImpl; +import org.apache.ignite.internal.processors.query.QueryEntityEx; +import org.apache.ignite.internal.util.typedef.internal.U; +import org.apache.ignite.marshaller.Marshaller; +import org.apache.ignite.plugin.extensions.communication.Message; +import org.apache.ignite.plugin.extensions.communication.MessageFactory; +import org.apache.ignite.plugin.extensions.communication.MessageFactoryProvider; +import org.apache.ignite.plugin.extensions.communication.MessageSerializer; +import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest; +import org.junit.Test; + +import static org.apache.ignite.marshaller.Marshallers.jdk; +import static org.apache.ignite.spi.communication.tcp.TcpCommunicationSpi.makeMessageType; + +/** Test for serialization round-trip of {@link QueryEntityMessage} and {@link QueryEntityExMessage}. */ +public class QueryEntityMessageSerializationTest extends GridCommonAbstractTest { + /** Error suffix. */ + public static final String ERROR_SUFFIX = " count is not equal to the expected fields count. " + + "Has the number of fields in the `QueryEntity` or `QueryEntityEx` classes changed?"; + + /** */ + private final Marshaller marsh = jdk(); + + /** */ + private final MessageFactory msgFactory = new IgniteMessageFactoryImpl( + new MessageFactoryProvider[] {new CoreMessagesProvider(marsh, marsh, U.gridClassLoader())}); + + /** */ + @Test + public void testQueryEntity() throws Exception { + QueryEntity entity = queryEntity(); + + assertEquals(entity, serializeAndDeserialize(entity, serializableFieldsCount(QueryEntity.class))); + } + + /** */ + @Test + public void testQueryEntityEx() throws Exception { + QueryEntityEx entity = queryEntityEx(); + + long expReadWriteCnt = serializableFieldsCount(QueryEntityEx.class) - + /* QueryEntityEx duplicates 'notNullFields', but 'QueryEntityExMessage' doesn't */ 1; + + QueryEntity res = serializeAndDeserialize(entity, expReadWriteCnt); + + assertTrue(res instanceof QueryEntityEx); + assertEquals(entity, res); + + // Not part of QueryEntityEx.equals(), so assert it explicitly. + assertEquals(entity.fillAbsentPKsWithDefaults(), ((QueryEntityEx)res).fillAbsentPKsWithDefaults()); + } Review Comment: One last ask: a regression guard for the issue fixed in fa303631 (`F.transform` in `entityBase()`). Without it, a future "optimization" back to `F.viewReadOnly` would silently reintroduce the `NotSerializableException: QueryIndexMessage` on `cache_data.dat` / join data — nothing else in the suite exercises JDK-marshalling of a restored entity. On the pre-fix revision this test fails with exactly that exception; on the current head it passes (verified locally, 3/3 green): ```suggestion } /** Restored entity ends up in JDK-marshalled containers (CacheConfiguration in 'cache_data.dat', join data). */ @Test public void testRestoredEntityIsJdkSerializable() throws Exception { QueryEntityMessage msg = writeAndReadBack( new QueryEntityMessage(queryEntity()), serializableFieldsCount(QueryEntity.class)); U.marshal(marsh, msg.toEntity()); } ``` ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/operation/SchemaAddQueryEntityOperation.java: ########## @@ -71,42 +66,34 @@ public SchemaAddQueryEntityOperation( this.entities = entities; this.qryParallelism = qryParallelism; this.sqlEscape = sqlEscape; + + entitiesMsgs = F.viewReadOnly(entities, this::makeEntityMessage); Review Comment: Agreed with the offline argument: mutation of the source collection would be a caller bug and shouldn't be defended against here — and the pre-PR code marshalled the live collection at `prepareMarshal` time anyway, so a construction-time snapshot never existed. Withdrawing, resolving. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/operation/SchemaAddQueryEntityOperation.java: ########## @@ -71,42 +66,34 @@ public SchemaAddQueryEntityOperation( this.entities = entities; this.qryParallelism = qryParallelism; this.sqlEscape = sqlEscape; + + entitiesMsgs = F.viewReadOnly(entities, this::makeEntityMessage); } - /** - * @return Collection of query entities. - */ + /** @return Collection of query entities. */ public Collection<QueryEntity> entities() { + if (entities == null) + entities = F.viewReadOnly(entitiesMsgs, QueryEntityMessage::toEntity); Review Comment: Fixed in fa303631, thanks — the getter contract (stable materialized collection, as the pre-PR `finishUnmarshal` provided) is restored. Resolving. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessage.java: ########## @@ -0,0 +1,154 @@ +/* + * 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.schema.message; + +import java.util.Collection; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.apache.ignite.IgniteCheckedException; +import org.apache.ignite.cache.QueryEntity; +import org.apache.ignite.internal.MarshallableMessage; +import org.apache.ignite.internal.Order; +import org.apache.ignite.internal.cache.query.QueryIndexMessage; +import org.apache.ignite.internal.util.typedef.F; +import org.apache.ignite.internal.util.typedef.internal.U; +import org.apache.ignite.marshaller.Marshaller; + +/** + * Message for {@link QueryEntity} transfer. + */ +public class QueryEntityMessage implements MarshallableMessage { + /** Key type. */ + @Order(0) + String keyType; + + /** Value type. */ + @Order(1) + String valType; + + /** Key name. */ + @Order(2) + String keyFieldName; + + /** Value name. */ + @Order(3) + String valFieldName; + + /** Fields available for query. */ + @Order(4) + LinkedHashMap<String, String> fields; + + /** Set of field names that belong to the key. */ + @Order(5) + String[] keyFields; + + /** Aliases. */ + @Order(6) + Map<String, String> aliases; + + /** Collection of query indexes. */ + @Order(7) + Collection<QueryIndexMessage> idxs; + + /** Table name. */ + @Order(8) + String tableName; + + /** + * Fields that must have non-null value. + * NB: Used in serde of both QueryEntity and QueryEntityEx in order to avoid duplication. + */ + @Order(9) + Set<String> notNullFields; + + /** Fields default values. */ + Map<String, Object> dfltFieldValues; + + /** Serialized form of {@link #dfltFieldValues}. */ + @Order(10) + byte[] dfltFieldValuesBytes; + + /** Precision(Maximum length) for fields. */ + @Order(11) + Map<String, Integer> fieldsPrecision; + + /** Scale for fields. */ + @Order(12) + Map<String, Integer> fieldsScale; + + /** */ + public QueryEntityMessage() { } + + /** @param qryEntity Original {@link QueryEntity}. */ + public QueryEntityMessage(QueryEntity qryEntity) { + keyType = qryEntity.getKeyType(); + valType = qryEntity.getValueType(); + + keyFieldName = qryEntity.getKeyFieldName(); + valFieldName = qryEntity.getValueFieldName(); + + fields = qryEntity.getFields(); + aliases = qryEntity.getAliases(); + + if (!F.isEmpty(qryEntity.getKeyFields())) + keyFields = qryEntity.getKeyFields().toArray(U.EMPTY_STRS); + + if (!F.isEmpty(qryEntity.getIndexes())) + idxs = F.viewReadOnly(qryEntity.getIndexes(), QueryIndexMessage::new); Review Comment: Same reasoning as the constructor thread: write-side view is fine under the "mutation is a caller bug" contract. Withdrawing, resolving. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessage.java: ########## @@ -0,0 +1,154 @@ +/* + * 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.schema.message; + +import java.util.Collection; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.apache.ignite.IgniteCheckedException; +import org.apache.ignite.cache.QueryEntity; +import org.apache.ignite.internal.MarshallableMessage; +import org.apache.ignite.internal.Order; +import org.apache.ignite.internal.cache.query.QueryIndexMessage; +import org.apache.ignite.internal.util.typedef.F; +import org.apache.ignite.internal.util.typedef.internal.U; +import org.apache.ignite.marshaller.Marshaller; + +/** + * Message for {@link QueryEntity} transfer. + */ +public class QueryEntityMessage implements MarshallableMessage { + /** Key type. */ + @Order(0) + String keyType; + + /** Value type. */ + @Order(1) + String valType; + + /** Key name. */ + @Order(2) + String keyFieldName; + + /** Value name. */ + @Order(3) + String valFieldName; + + /** Fields available for query. */ + @Order(4) + LinkedHashMap<String, String> fields; + + /** Set of field names that belong to the key. */ + @Order(5) + String[] keyFields; + + /** Aliases. */ + @Order(6) + Map<String, String> aliases; + + /** Collection of query indexes. */ + @Order(7) + Collection<QueryIndexMessage> idxs; + + /** Table name. */ + @Order(8) + String tableName; + + /** + * Fields that must have non-null value. + * NB: Used in serde of both QueryEntity and QueryEntityEx in order to avoid duplication. + */ + @Order(9) + Set<String> notNullFields; + + /** Fields default values. */ + Map<String, Object> dfltFieldValues; + + /** Serialized form of {@link #dfltFieldValues}. */ + @Order(10) + byte[] dfltFieldValuesBytes; + + /** Precision(Maximum length) for fields. */ + @Order(11) + Map<String, Integer> fieldsPrecision; + + /** Scale for fields. */ + @Order(12) + Map<String, Integer> fieldsScale; + + /** */ + public QueryEntityMessage() { } + + /** @param qryEntity Original {@link QueryEntity}. */ + public QueryEntityMessage(QueryEntity qryEntity) { + keyType = qryEntity.getKeyType(); + valType = qryEntity.getValueType(); + + keyFieldName = qryEntity.getKeyFieldName(); + valFieldName = qryEntity.getValueFieldName(); + + fields = qryEntity.getFields(); + aliases = qryEntity.getAliases(); + + if (!F.isEmpty(qryEntity.getKeyFields())) + keyFields = qryEntity.getKeyFields().toArray(U.EMPTY_STRS); + + if (!F.isEmpty(qryEntity.getIndexes())) + idxs = F.viewReadOnly(qryEntity.getIndexes(), QueryIndexMessage::new); + + tableName = qryEntity.getTableName(); + + notNullFields = qryEntity.getNotNullFields(); + dfltFieldValues = qryEntity.getDefaultFieldValues(); + fieldsPrecision = qryEntity.getFieldsPrecision(); + fieldsScale = qryEntity.getFieldsScale(); + } + + /** @return Original {@link QueryEntity}. */ + public QueryEntity toEntity() { + return new QueryEntity() + .setKeyType(keyType) + .setValueType(valType) + .setKeyFieldName(keyFieldName) + .setValueFieldName(valFieldName) + .setFields(fields) + .setKeyFields(!F.isEmpty(keyFields) ? new LinkedHashSet<>(List.of(keyFields)) : null) + .setAliases(aliases) + .setIndexes(!F.isEmpty(idxs) ? F.viewReadOnly(idxs, QueryIndexMessage::queryIndex) : null) Review Comment: Fixed in fa303631, thanks. For the record, this one was load-bearing, not hygiene: with the view, the restored entity failed JDK-marshalling (`NotSerializableException: QueryIndexMessage`, no longer `Serializable` after this PR) once it reached `CacheConfiguration.qryEntities` via `DynamicCacheDescriptor.schemaChangeFinish()` -> `CU.patchCacheConfiguration()` — i.e. `cache_data.dat` on persistent clusters and `CacheData` join discovery data. Suggested a regression test in a separate comment. Resolving. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/operation/SchemaAddQueryEntityOperation.java: ########## @@ -71,42 +66,34 @@ public SchemaAddQueryEntityOperation( this.entities = entities; this.qryParallelism = qryParallelism; this.sqlEscape = sqlEscape; + + entitiesMsgs = F.viewReadOnly(entities, this::makeEntityMessage); } - /** - * @return Collection of query entities. - */ + /** @return Collection of query entities. */ public Collection<QueryEntity> entities() { + if (entities == null) + entities = F.viewReadOnly(entitiesMsgs, QueryEntityMessage::toEntity); + return entities; } - /** - * @return Query parallelism. - */ + /** @return Query parallelism. */ public int queryParallelism() { return qryParallelism; } - /** - * @return Sql escape flag. - */ + /** @return Sql escape flag. */ public boolean isSqlEscape() { return sqlEscape; } - /** {@inheritDoc} */ - @Override public void prepareMarshal(Marshaller marsh) throws IgniteCheckedException { - if (entities != null) - qryEntitiesBytes = U.marshal(marsh, entities); - } - - /** {@inheritDoc} */ - @Override public void finishUnmarshal(Marshaller marsh, ClassLoader clsLdr) throws IgniteCheckedException { - if (qryEntitiesBytes != null) { - entities = U.unmarshal(marsh, qryEntitiesBytes, clsLdr); - - qryEntitiesBytes = null; - } + /** + * @param qryEntity Query entity. + * @return The appropriate query entity message. + */ + private QueryEntityMessage makeEntityMessage(QueryEntity qryEntity) { Review Comment: Optional as stated — fine to skip. Resolving. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityExMessage.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.schema.message; + +import org.apache.ignite.cache.QueryEntity; +import org.apache.ignite.internal.Order; +import org.apache.ignite.internal.processors.query.QueryEntityEx; + +/** Message for {@link QueryEntityEx} transfer. */ +public class QueryEntityExMessage extends QueryEntityMessage { + /** Whether to preserve order specified by 'keyFields' or not. */ + @Order(0) + boolean preserveKeysOrder; + + /** Whether a primary key should be autocreated or not. */ + @Order(1) + boolean implicitPk; + + /** Whether absent PK parts should be filled with defaults or not. */ + @Order(2) + boolean fillAbsentPKsWithDefaults; + + /** INLINE_SIZE for PK index. */ + @Order(3) + int pkInlineSize; + + /** INLINE_SIZE for affinity field index. */ + @Order(4) + int affKeyInlineSize; + + /** Whether query entity was created by SQL. */ + @Order(5) + boolean sql; + + /** */ + public QueryEntityExMessage() { } + + /** + * @param qryEntity Original {@link QueryEntity}. + */ + public QueryEntityExMessage(QueryEntityEx qryEntity) { + super(qryEntity); + + preserveKeysOrder = qryEntity.isPreserveKeysOrder(); + implicitPk = qryEntity.implicitPk(); + fillAbsentPKsWithDefaults = qryEntity.fillAbsentPKsWithDefaults(); + + pkInlineSize = qryEntity.getPrimaryKeyInlineSize() != null ? qryEntity.getPrimaryKeyInlineSize() : -1; + + affKeyInlineSize = qryEntity.getAffinityKeyInlineSize() != null ? qryEntity.getAffinityKeyInlineSize() : -1; + + sql = qryEntity.sql(); + } + + /** {@inheritDoc} */ + @Override public QueryEntity toEntity() { + QueryEntity baseEntity = super.toEntity(); + + // We should nullify '_notNullFields' field of base entity, + // because 'QueryEntityEx' uses extra 'notNullFields' field for storing of not null fields. + baseEntity.setNotNullFields(null); Review Comment: Adopted as `entityBase()` in fa303631, thanks. Resolving. -- 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]
