anton-vinogradov commented on code in PR #13236: URL: https://github.com/apache/ignite/pull/13236#discussion_r3475280436
########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessage.java: ########## @@ -0,0 +1,155 @@ +/* + * 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. Can be used in field list to denote the key as a whole. */ + @Order(2) + String keyFieldName; + + /** Value name. Can be used in field list to denote the entire value. */ + @Order(3) + String valFieldName; + + /** Fields available for query. A map from field name to type name. */ + @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: DO NOT remove underscore to avoid clashes with QueryEntityEx. */ + @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) { Review Comment: **Guard the hand-written clone against drift.** `QueryEntityMessage`/`QueryEntityExMessage` duplicate `QueryEntity`/`QueryEntityEx` field-by-field in two directions, with no compile-time link to the source classes. When someone later adds a field to the public `QueryEntity`, it's easy to miss the twin and silently drop it on the wire. The mapping is complete today — to keep it complete, consider a small reflection-based parity test asserting every data field of `QueryEntity`/`QueryEntityEx` is represented here. ########## modules/core/src/main/java/org/apache/ignite/internal/CoreMessagesProvider.java: ########## @@ -582,7 +584,9 @@ public CoreMessagesProvider(Marshaller dfltMarsh, Marshaller schemaAwareMarsh, C withNoSchema(CacheContinuousQueryBatchAck.class); withSchema(CacheContinuousQueryEntry.class); withNoSchema(QueryInlineSizesDataBagItem.class); - withSchema(QueryProposalsDataBagItem.class); + withNoSchema(QueryProposalsDataBagItem.class); Review Comment: Same reasoning as the `AuthentificationDataBagItem` flip below: `QueryProposalsDataBagItem` is a plain `Message`, so `withSchema` vs `withNoSchema` has no functional effect here (its registered marshaller is unused). Harmless, but it's diff noise — either drop it or move both flips to a separate cleanup commit with a one-line rationale. ########## modules/core/src/main/java/org/apache/ignite/internal/CoreMessagesProvider.java: ########## @@ -636,7 +640,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 change looks out of scope for IGNITE-28767 — `AuthentificationDataBagItem` is auth/users, not QueryEntity. It also appears to be a no-op: it's a plain (non-`MarshallableMessage`) `Message`, so the registered marshaller is never used — nested messages resolve their own marshaller by `directType`, and the generated marshaller only takes a `Marshaller` when `marshallable || hasMarshalled`. Could we drop it from this PR? ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessage.java: ########## @@ -0,0 +1,155 @@ +/* + * 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 { Review Comment: **Missing tests (blocker, imo).** There's no test exercising this serialization path. Could you add a round-trip test: build a `QueryEntity` and a `QueryEntityEx` with all fields populated (incl. a non-empty `defaultFieldValues` of a non-trivial type), convert to `*Message`, run prepare/finish marshal, `toEntity()` back, and assert `equals`? `QueryEntity`/`QueryEntityEx` already have correct `equals`/`hashCode`, so this is cheap and would protect the whole change. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessage.java: ########## @@ -0,0 +1,155 @@ +/* + * 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. Can be used in field list to denote the key as a whole. */ + @Order(2) + String keyFieldName; + + /** Value name. Can be used in field list to denote the entire value. */ + @Order(3) + String valFieldName; + + /** Fields available for query. A map from field name to type name. */ + @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: DO NOT remove underscore to avoid clashes with QueryEntityEx. */ + @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) + .setTableName(tableName) + .setNotNullFields(notNullFields) + .setDefaultFieldValues(dfltFieldValues) + .setFieldsPrecision(fieldsPrecision) + .setFieldsScale(fieldsScale); + } + + /** {@inheritDoc} */ + @Override public void prepareMarshal(Marshaller marsh) throws IgniteCheckedException { + if (!F.isEmpty(dfltFieldValues)) + dfltFieldValuesBytes = U.marshal(marsh, dfltFieldValues); Review Comment: Worth a line in the PR description: `defaultFieldValues` are arbitrary user objects that can't be a `Message`, so they still go through `U.marshal` (Java serialization). That's unavoidable, but it means this particular payload isn't actually moved off Java serialization by the change — and it's exactly the part the round-trip test should cover with a non-trivial value. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/operation/SchemaAddQueryEntityOperation.java: ########## @@ -19,26 +19,20 @@ import java.util.Collection; import java.util.UUID; -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.util.typedef.internal.U; -import org.apache.ignite.marshaller.Marshaller; +import org.apache.ignite.internal.processors.query.QueryEntityEx; +import org.apache.ignite.internal.processors.query.schema.message.QueryEntityExMessage; +import org.apache.ignite.internal.processors.query.schema.message.QueryEntityMessage; +import org.apache.ignite.internal.util.typedef.F; /** * Enabling indexing on cache operation. */ -public class SchemaAddQueryEntityOperation extends SchemaAbstractOperation implements MarshallableMessage { - /** */ - private static final long serialVersionUID = 0L; - - /** */ - private Collection<QueryEntity> entities; - - /** Serialized form of query entities. */ +public class SchemaAddQueryEntityOperation extends SchemaAbstractOperation { + /** Query entities. */ @Order(0) Review Comment: Just flagging (not blocking): this is the only change in the PR that alters the on-wire representation of `@Order(0)` (previously a Java-serialized `byte[]`, now a `Collection<Message>`). It's safe for OSS, where mixed-version discovery isn't supported (old nodes are refused at join), but it diverges from the IEP-132 "binary representation unchanged" property the rest of the series relies on. Worth a note in the PR description as an intentional format-breaking change so the rolling-upgrade track is aware. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessage.java: ########## @@ -0,0 +1,155 @@ +/* + * 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. Can be used in field list to denote the key as a whole. */ + @Order(2) + String keyFieldName; + + /** Value name. Can be used in field list to denote the entire value. */ + @Order(3) + String valFieldName; + + /** Fields available for query. A map from field name to type name. */ + @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: DO NOT remove underscore to avoid clashes with QueryEntityEx. */ + @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) Review Comment: Nit: a non-null-but-empty `keyFields`/`idxs` round-trips to `null` (empty-check in, `null` out). Almost certainly harmless since the defaults are `null`, but worth being aware of if an `equals`-based test is added. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessage.java: ########## @@ -0,0 +1,155 @@ +/* + * 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. Can be used in field list to denote the key as a whole. */ + @Order(2) + String keyFieldName; + + /** Value name. Can be used in field list to denote the entire value. */ + @Order(3) + String valFieldName; + + /** Fields available for query. A map from field name to type name. */ + @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: DO NOT remove underscore to avoid clashes with QueryEntityEx. */ Review Comment: Nit: this comment was copied from `QueryEntity`, but here the field is `notNullFields` (no underscore) and `QueryEntityExMessage` doesn't redeclare it, so the "DO NOT remove underscore" note no longer applies and is misleading. Suggest dropping it or replacing with a plain description. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/query/schema/operation/SchemaAbstractOperation.java: ########## @@ -26,10 +25,7 @@ /** * Abstract operation on schema. */ -public abstract class SchemaAbstractOperation implements Serializable, Message { - /** */ - private static final long serialVersionUID = 0L; - +public abstract class SchemaAbstractOperation implements Message { Review Comment: Follow-up, non-blocking: now that the operation isn't `Serializable`, its discovery container (`SchemaAbstractDiscoveryMessage`, `SchemaProposeDiscoveryMessage`) still declares `implements Serializable` / `serialVersionUID`. AFAICT it's vestigial — both TCP and ZK discovery marshal custom messages via the Message framework, not `JdkMarshaller`, so nothing Java-serializes the op. For consistency it'd be nice to drop the dead `Serializable` from those containers too, in a separate cleanup. -- 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]
