anton-vinogradov commented on code in PR #13236:
URL: https://github.com/apache/ignite/pull/13236#discussion_r3483055304
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/operation/SchemaAddQueryEntityOperation.java:
##########
@@ -68,7 +62,7 @@ public SchemaAddQueryEntityOperation(
boolean sqlEscape
) {
super(opId, cacheName, schemaName);
- this.entities = entities;
+ this.entities = F.viewReadOnly(entities, this::makeEntityMessage);
Review Comment:
`entities` is stored as a lazy `F.viewReadOnly` over the original
`Collection<QueryEntity>`, and `entities()` (line 74) layers a second lazy view
on top. On the originating node `entities()` then runs `QueryEntity ->
QueryEntityMessage -> QueryEntity` on **every** iteration — returning
reconstructed copies rather than the originals, re-doing the conversion each
call (it's invoked from a number of places: `GridQueryProcessor`,
`QuerySchema`, `GridCacheUtils`, ...), and the view also pins the source
collection. Consider materializing once, e.g. `this.entities = new
ArrayList<>(F.viewReadOnly(entities, this::makeEntityMessage));`. Non-blocking.
##########
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 orther to
avoid duplication.
Review Comment:
Nit: typo — "in orther to" -> "in order to".
##########
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());
+ }
+
+ /**
+ * @param src Source entity.
+ * @param expReadsWritesCnt Expected count of field reads and writes.
+ *
+ * @return Entity read during a full serde round-trip.
+ */
+ private QueryEntity serializeAndDeserialize(QueryEntity src, long
expReadsWritesCnt) {
+ QueryEntityMessage msg = src instanceof QueryEntityEx
+ ? new QueryEntityExMessage((QueryEntityEx)src) : new
QueryEntityMessage(src);
+
+ return writeAndReadBack(msg, expReadsWritesCnt).toEntity();
+ }
+
+ /**
+ * @param cls Class of an object.
+ */
+ private long serializableFieldsCount(Class<?> cls) {
Review Comment:
The field-count guard is a nice touch. One gap: it covers
`QueryEntity`/`QueryEntityEx` but not the nested `QueryIndex` ->
`QueryIndexMessage` mapping — adding a field to `QueryIndex` wouldn't change
any counted total. Could be worth extending the same `serializableFieldsCount`
check to `QueryIndex`, or noting the limitation.
##########
modules/core/src/main/java/org/apache/ignite/internal/CoreMessagesProvider.java:
##########
@@ -641,7 +645,7 @@ public CoreMessagesProvider(Marshaller dfltMarsh,
Marshaller schemaAwareMarsh, C
withNoSchema(UserAuthenticateRequestMessage.class);
withNoSchema(UserAuthenticateResponseMessage.class);
withNoSchema(TcpDiscoveryAuthFailedMessage.class);
- withSchema(AuthentificationDataBagItem.class);
+ withNoSchema(AuthentificationDataBagItem.class);
Review Comment:
This `withSchema` -> `withNoSchema` flip is still here and is unrelated to
QueryEntity (it's auth/users). For `QueryProposalsDataBagItem` you noted it's
#13157 tech debt — if this one is the same kind of mis-registration cleanup,
could you say so (and ideally call both out in the description)? Otherwise I'd
drop it from this ticket. It looks like a no-op anyway (plain `Message`, so the
registered marshaller is unused).
--
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]
