Copilot commented on code in PR #7471: URL: https://github.com/apache/ignite-3/pull/7471#discussion_r2740333624
########## modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/SharedStateMessageConverterTest.java: ########## @@ -0,0 +1,140 @@ +/* + * 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.sql.engine.exec; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; + +import java.time.Duration; +import java.time.Instant; +import java.time.LocalDate; +import java.time.Period; +import java.util.EnumSet; +import java.util.List; +import java.util.Random; +import java.util.Set; +import org.apache.ignite.internal.schema.SchemaTestUtils; +import org.apache.ignite.internal.sql.engine.message.SharedStateMessage; +import org.apache.ignite.internal.sql.engine.util.TypeUtils; +import org.apache.ignite.internal.type.NativeType; +import org.apache.ignite.sql.ColumnType; +import org.hamcrest.Matchers; +import org.jetbrains.annotations.Nullable; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +/** + * Tests for class {@link SharedStateMessageConverter}. + */ +public class SharedStateMessageConverterTest { + private Random rnd; + + /** + * Initialization. + */ + @BeforeEach + public void initRandom() { + long seed = System.currentTimeMillis(); + + System.out.println("Using seed: " + seed + "L;"); + + rnd = new Random(seed); Review Comment: This test prints the random seed to stdout via `System.out.println`, which can add noise to CI logs. Prefer using the project/test logger (or only printing on failure) to keep test output clean while still making failures reproducible. ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/SharedStateMessageConverter.java: ########## @@ -0,0 +1,178 @@ +/* + * 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.sql.engine.exec; + +import it.unimi.dsi.fastutil.longs.Long2ObjectMap; +import it.unimi.dsi.fastutil.longs.Long2ObjectOpenHashMap; +import java.math.BigDecimal; +import java.math.BigInteger; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Map; +import java.util.UUID; +import org.apache.calcite.avatica.util.ByteString; +import org.apache.ignite.internal.network.NetworkMessage; +import org.apache.ignite.internal.sql.engine.message.SharedStateMessage; +import org.apache.ignite.internal.sql.engine.message.SqlQueryMessageGroup; +import org.apache.ignite.internal.sql.engine.message.SqlQueryMessagesFactory; +import org.apache.ignite.internal.sql.engine.message.field.SingleValueMessage; +import org.apache.ignite.internal.util.IgniteUtils; +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.Nullable; + +/** + * Converter between {@link SharedState} and {@link SharedStateMessage}. + */ +public class SharedStateMessageConverter { + /** Message factory. */ + private static final SqlQueryMessagesFactory MESSAGE_FACTORY = new SqlQueryMessagesFactory(); + + @Contract("null -> null; !null -> !null") + static @Nullable SharedStateMessage toMessage(@Nullable SharedState state) { + if (state == null) { + return null; + } + + Long2ObjectMap<Object> correlations = state.correlations(); + Map<Long, NetworkMessage> result = IgniteUtils.newHashMap(correlations.size()); + + for (Long2ObjectMap.Entry<Object> entry : correlations.long2ObjectEntrySet()) { + SingleValueMessage<?> msg = toSingleValueMessage(entry.getValue()); + + result.put(entry.getLongKey(), msg); + } + + return MESSAGE_FACTORY.sharedStateMessage() + .sharedState(result) + .build(); + } + + @Contract("null -> null; !null -> !null") + static @Nullable SharedState fromMessage(@Nullable SharedStateMessage sharedStateMessage) { + if (sharedStateMessage == null) { + return null; + } + + int size = sharedStateMessage.sharedState().size(); + Long2ObjectMap<Object> correlations = new Long2ObjectOpenHashMap<>(size); + + for (Map.Entry<Long, NetworkMessage> e : sharedStateMessage.sharedState().entrySet()) { + NetworkMessage networkMessage = e.getValue(); + + if (!(networkMessage instanceof SingleValueMessage)) { + throw new IllegalArgumentException("Unexpected message type " + + "[type=" + networkMessage.messageType() + ", class=" + networkMessage.getClass() + ']'); + } + + SingleValueMessage<Object> singleFieldMessage = ((SingleValueMessage<Object>) networkMessage); + + correlations.put(e.getKey().longValue(), extractFieldValue(singleFieldMessage)); + } + + return new SharedState(correlations); + } + + private static @Nullable Object extractFieldValue(SingleValueMessage<Object> msg) { + Object value = msg.value(); + + if (value == null) { + return null; + } + + switch (msg.messageType()) { + case SqlQueryMessageGroup.BYTE_ARRAY_FIELD_MESSAGE: + return new ByteString((byte[]) value); + + case SqlQueryMessageGroup.DECIMAL_FIELD_MESSAGE: + return decimalFromBytes((byte[]) value); + + default: + return value; + } Review Comment: `extractFieldValue` returns `null` early only when `msg.value()` is `null`. For `NullValueMessage`, this currently works because the builder leaves `value` unset, but if a NULL_FIELD_MESSAGE ever arrives with a non-null payload (buggy sender, version skew, etc.), it would incorrectly propagate that value. Consider explicitly handling `SqlQueryMessageGroup.NULL_FIELD_MESSAGE` in the switch (returning `null` regardless of payload, or rejecting non-null payloads) to make the converter semantics robust. ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/message/SharedStateMessage.java: ########## @@ -0,0 +1,30 @@ +/* + * 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.sql.engine.message; + +import java.util.Map; +import org.apache.ignite.internal.network.NetworkMessage; +import org.apache.ignite.internal.network.annotations.Transferable; + +/** + * A message that contains map with correlations. Review Comment: Minor grammar in the Javadoc: "contains map" → "contains a map". ```suggestion * A message that contains a map with correlations. ``` ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/SharedState.java: ########## @@ -17,30 +17,40 @@ package org.apache.ignite.internal.sql.engine.exec; -import static org.apache.ignite.internal.sql.engine.util.Commons.checkRange; - -import java.io.Serializable; -import org.apache.ignite.internal.sql.engine.util.Commons; +import it.unimi.dsi.fastutil.longs.Long2ObjectMap; +import it.unimi.dsi.fastutil.longs.Long2ObjectMaps; +import it.unimi.dsi.fastutil.longs.Long2ObjectOpenHashMap; +import org.jetbrains.annotations.Nullable; /** * This class represents the volatile state that may be propagated from parent to its children * during rewind. */ -public class SharedState implements Serializable { - private static final long serialVersionUID = 42L; +public class SharedState { + private final Long2ObjectMap<Object> correlations; + + public SharedState() { + this(new Long2ObjectOpenHashMap<>()); + } - private Object[] correlations = new Object[16]; + SharedState(Long2ObjectMap<Object> correlations) { + this.correlations = correlations; + } /** * Gets correlated value. * * @param id Correlation ID. * @return Correlated value. Review Comment: The Javadoc still describes `id` as a "Correlation ID", but the new API uses a `long` key that encodes correlation id + field index (as reflected in `SqlEvaluationContext#correlatedVariable(long)` and the new callers). Please update the parameter docs to avoid confusion/misuse. -- 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]
