Copilot commented on code in PR #7406:
URL: https://github.com/apache/ignite-3/pull/7406#discussion_r2707214532
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/row/RowAssembler.java:
##########
@@ -124,22 +124,22 @@ public RowAssembler appendValue(@Nullable Object val)
throws SchemaMismatchExcep
return appendBoolean((boolean) val);
}
case INT8: {
- return appendByte((byte) val);
+ return appendByte(((Number) val).byteValue());
}
case INT16: {
- return appendShort((short) val);
+ return appendShort(((Number) val).shortValue());
}
case INT32: {
- return appendInt((int) val);
+ return appendInt(((Number) val).intValue());
}
case INT64: {
- return appendLong((long) val);
+ return appendLong(((Number) val).longValue());
}
case FLOAT: {
- return appendFloat((float) val);
+ return appendFloat(((Number) val).floatValue());
}
case DOUBLE: {
- return appendDouble((double) val);
+ return appendDouble(((Number) val).doubleValue());
Review Comment:
The code casts to Number without prior validation, which can result in a
ClassCastException if the value is not a Number. While Column.validate is
called before this code in the normal flow, this creates a fragile dependency.
Consider adding a type check or documenting that this method assumes validation
has already been performed by Column.validate.
##########
modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ClientBinaryTupleUtils.java:
##########
@@ -288,6 +288,165 @@ public static void appendValue(BinaryTupleBuilder
builder, ColumnType type, Stri
}
}
+ private static void appendByteValue(BinaryTupleBuilder builder, Object
val) {
+ if (val instanceof Byte) {
+ builder.appendByte((byte) val);
+ return;
+ }
+
+ if (val instanceof Short) {
+ short shortVal = (short) val;
+ byte byteVal = (byte) shortVal;
+
+ if (shortVal == byteVal) {
+ builder.appendByte(byteVal);
+ return;
+ }
+ }
+
+ if (val instanceof Integer) {
+ int intVal = (int) val;
+ byte byteVal = (byte) intVal;
+
+ if (intVal == byteVal) {
+ builder.appendByte(byteVal);
+ return;
+ }
+ }
+
+ if (val instanceof Long) {
+ long longVal = (long) val;
+ byte byteVal = (byte) longVal;
+
+ if (longVal == byteVal) {
+ builder.appendByte(byteVal);
+ return;
+ }
+ }
+
+ throw new ClassCastException("Cannot cast to byte: " + val.getClass());
+ }
+
+ private static void appendShortValue(BinaryTupleBuilder builder, Object
val) {
+ if (val instanceof Short) {
+ builder.appendShort((short) val);
+ return;
+ }
+
+ if (val instanceof Byte) {
+ builder.appendShort((byte) val);
+ return;
+ }
+
+ if (val instanceof Integer) {
+ int intVal = (int) val;
+ short shortVal = (short) intVal;
+
+ if (intVal == shortVal) {
+ builder.appendShort(shortVal);
+ return;
+ }
+ }
+
+ if (val instanceof Long) {
+ long longVal = (long) val;
+ short shortVal = (short) longVal;
+
+ if (longVal == shortVal) {
+ builder.appendShort(shortVal);
+ return;
+ }
+ }
+
+ throw new ClassCastException("Cannot cast to short: " +
val.getClass());
+ }
+
+ private static void appendIntValue(BinaryTupleBuilder builder, Object val)
{
+ if (val instanceof Integer) {
+ builder.appendInt((int) val);
+ return;
+ }
+
+ if (val instanceof Short) {
+ builder.appendInt((short) val);
+ return;
+ }
+
+ if (val instanceof Byte) {
+ builder.appendInt((byte) val);
+ return;
+ }
+
+ if (val instanceof Long) {
+ long longVal = (long) val;
+ int intVal = (int) longVal;
+
+ if (longVal == intVal) {
+ builder.appendInt(intVal);
+ return;
+ }
+ }
+
+ throw new ClassCastException("Cannot cast to int: " + val.getClass());
+ }
+
+ private static void appendLongValue(BinaryTupleBuilder builder, Object
val) {
+ if (val instanceof Integer) {
+ builder.appendLong((int) val);
+ return;
+ }
+
+ if (val instanceof Short) {
+ builder.appendLong((short) val);
+ return;
+ }
+
+ if (val instanceof Byte) {
+ builder.appendLong((byte) val);
+ return;
+ }
+
+ if (val instanceof Long) {
+ builder.appendLong((long) val);
+ return;
+ }
+
+ throw new ClassCastException("Cannot cast to long: " + val.getClass());
+ }
+
+ private static void appendFloatValue(BinaryTupleBuilder builder, Object
val) {
+ if (val instanceof Float) {
+ builder.appendFloat((float) val);
+ return;
+ }
+
+ if (val instanceof Double) {
+ double doubleVal = (double) val;
+ float floatVal = (float) doubleVal;
+
+ if (doubleVal == floatVal || Double.isNaN(doubleVal)) {
+ builder.appendFloat(floatVal);
+ return;
+ }
+ }
+
+ throw new ClassCastException("Cannot cast to float: " +
val.getClass());
+ }
+
+ private static void appendDoubleValue(BinaryTupleBuilder builder, Object
val) {
+ if (val instanceof Double) {
+ builder.appendDouble((double) val);
+ return;
+ }
+
+ if (val instanceof Float) {
+ builder.appendDouble((float) val);
+ return;
+ }
+
+ throw new ClassCastException("Cannot cast to double: " +
val.getClass());
+ }
Review Comment:
There is significant code duplication between
TupleTypeCastUtils.isCastAllowed and the appendXxxValue methods in
ClientBinaryTupleUtils. Both implement the same type casting logic with range
checks. Consider refactoring to reuse TupleTypeCastUtils in
ClientBinaryTupleUtils to avoid maintaining two separate implementations of the
same logic, which could lead to inconsistencies if one is updated without the
other.
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/table/ItKeyValueBinaryViewApiTest.java:
##########
@@ -582,6 +598,396 @@ public void schemaMismatch(TestCase testCase) {
);
}
+ @ParameterizedTest
+ @MethodSource("typeCastTestCases")
+ public void testWriteAsByte(TestCase testCase) {
+ KeyValueView<Tuple, Tuple> tbl = testCase.view();
+ String keyName = testCase.keyColumnName(0);
+ String valName = testCase.valColumnName(0);
+ Tuple key = Tuple.create().set(keyName, 1L);
+ ColumnType targetType = ColumnType.INT8;
+
+ // Put short value.
+ {
+ Tuple val = Tuple.create().set(valName, (short) Byte.MAX_VALUE);
+
+ tbl.put(null, key, val);
+ assertThat(tbl.get(null, key).byteValue(valName),
is(Byte.MAX_VALUE));
+
+ Tuple outOfRange = Tuple.create().set(valName, (short)
(Byte.MAX_VALUE + 1));
+ expectTypeMismatch(() -> tbl.put(null, key, outOfRange), valName,
targetType, ColumnType.INT16);
+ }
+
+ {
+ Tuple val = Tuple.create().set(valName, (short) Byte.MIN_VALUE);
+
+ tbl.put(null, key, val);
+ assertThat(tbl.get(null, key).byteValue(valName),
is(Byte.MIN_VALUE));
+
+ Tuple outOfRange = Tuple.create().set(valName, (short)
(Byte.MIN_VALUE - 1));
+ expectTypeMismatch(() -> tbl.put(null, key, outOfRange), valName,
targetType, ColumnType.INT16);
+ }
+
+ // Put int value.
+ {
+ Tuple val = Tuple.create().set(valName, (int) Byte.MAX_VALUE);
+
+ tbl.put(null, key, val);
+ assertThat(tbl.get(null, key).byteValue(valName),
is(Byte.MAX_VALUE));
+
+ Tuple outOfRange = Tuple.create().set(valName, Byte.MAX_VALUE + 1);
+ expectTypeMismatch(() -> tbl.put(null, key, outOfRange), valName,
targetType, ColumnType.INT32);
+ }
+
+ {
+ Tuple val = Tuple.create().set(valName, (short) Byte.MIN_VALUE);
+
+ tbl.put(null, key, val);
+ assertThat(tbl.get(null, key).byteValue(valName),
is(Byte.MIN_VALUE));
+
+ Tuple outOfRange = Tuple.create().set(valName, (short)
(Byte.MIN_VALUE - 1));
+ expectTypeMismatch(() -> tbl.put(null, key, outOfRange), valName,
targetType, ColumnType.INT16);
Review Comment:
Inconsistent test logic: this block is labeled "// Put int value" (line 631)
and should test putting an int value that equals Byte.MIN_VALUE, but line 643
incorrectly casts to short instead of int. This should be `(int)
Byte.MIN_VALUE` to match the test's intent and be consistent with line 633.
```suggestion
Tuple val = Tuple.create().set(valName, (int) Byte.MIN_VALUE);
tbl.put(null, key, val);
assertThat(tbl.get(null, key).byteValue(valName),
is(Byte.MIN_VALUE));
Tuple outOfRange = Tuple.create().set(valName, Byte.MIN_VALUE -
1);
expectTypeMismatch(() -> tbl.put(null, key, outOfRange),
valName, targetType, ColumnType.INT32);
```
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/table/ItKeyValueBinaryViewApiTest.java:
##########
@@ -582,6 +598,396 @@ public void schemaMismatch(TestCase testCase) {
);
}
+ @ParameterizedTest
+ @MethodSource("typeCastTestCases")
+ public void testWriteAsByte(TestCase testCase) {
+ KeyValueView<Tuple, Tuple> tbl = testCase.view();
+ String keyName = testCase.keyColumnName(0);
+ String valName = testCase.valColumnName(0);
+ Tuple key = Tuple.create().set(keyName, 1L);
+ ColumnType targetType = ColumnType.INT8;
+
+ // Put short value.
+ {
+ Tuple val = Tuple.create().set(valName, (short) Byte.MAX_VALUE);
+
+ tbl.put(null, key, val);
+ assertThat(tbl.get(null, key).byteValue(valName),
is(Byte.MAX_VALUE));
+
+ Tuple outOfRange = Tuple.create().set(valName, (short)
(Byte.MAX_VALUE + 1));
+ expectTypeMismatch(() -> tbl.put(null, key, outOfRange), valName,
targetType, ColumnType.INT16);
+ }
+
+ {
+ Tuple val = Tuple.create().set(valName, (short) Byte.MIN_VALUE);
+
+ tbl.put(null, key, val);
+ assertThat(tbl.get(null, key).byteValue(valName),
is(Byte.MIN_VALUE));
+
+ Tuple outOfRange = Tuple.create().set(valName, (short)
(Byte.MIN_VALUE - 1));
+ expectTypeMismatch(() -> tbl.put(null, key, outOfRange), valName,
targetType, ColumnType.INT16);
+ }
+
+ // Put int value.
+ {
+ Tuple val = Tuple.create().set(valName, (int) Byte.MAX_VALUE);
+
+ tbl.put(null, key, val);
+ assertThat(tbl.get(null, key).byteValue(valName),
is(Byte.MAX_VALUE));
+
+ Tuple outOfRange = Tuple.create().set(valName, Byte.MAX_VALUE + 1);
+ expectTypeMismatch(() -> tbl.put(null, key, outOfRange), valName,
targetType, ColumnType.INT32);
+ }
+
+ {
+ Tuple val = Tuple.create().set(valName, (short) Byte.MIN_VALUE);
+
+ tbl.put(null, key, val);
+ assertThat(tbl.get(null, key).byteValue(valName),
is(Byte.MIN_VALUE));
+
+ Tuple outOfRange = Tuple.create().set(valName, (short)
(Byte.MIN_VALUE - 1));
+ expectTypeMismatch(() -> tbl.put(null, key, outOfRange), valName,
targetType, ColumnType.INT16);
Review Comment:
Inconsistent test logic: this block is labeled "// Put int value" (line 631)
and should test putting an int value that is out of range, but line 648
incorrectly casts to short and expects INT16 type. This should be
`Byte.MIN_VALUE - 1` (as an int) and expect INT32 to match the test's intent
and be consistent with line 638.
```suggestion
Tuple val = Tuple.create().set(valName, (int) Byte.MIN_VALUE);
tbl.put(null, key, val);
assertThat(tbl.get(null, key).byteValue(valName),
is(Byte.MIN_VALUE));
Tuple outOfRange = Tuple.create().set(valName, Byte.MIN_VALUE -
1);
expectTypeMismatch(() -> tbl.put(null, key, outOfRange),
valName, targetType, ColumnType.INT32);
```
--
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]
