Copilot commented on code in PR #7319:
URL: https://github.com/apache/ignite-3/pull/7319#discussion_r2663961951
##########
modules/api/src/main/java/org/apache/ignite/table/TupleImpl.java:
##########
@@ -437,4 +437,26 @@ public String toString() {
return b.toString();
}
+
+ private <T> T valueNotNull(int columnIndex) {
+ T value = value(columnIndex);
+
+ if (value == null) {
+ throw new NullPointerException("The value of field at index " +
columnIndex
+ + " is null and cannot be converted to a primitive data
type.");
+ }
Review Comment:
The error messages in TupleImpl are hardcoded and inconsistent with the
error message constants defined in IgniteUtils.NULL_TO_PRIMITIVE_ERROR_MESSAGE
and IgniteUtils.NULL_TO_PRIMITIVE_NAMED_ERROR_MESSAGE. While TupleImpl is in
the public API module and cannot depend on internal modules, the error messages
should be consistent across all implementations for a better user experience.
Consider defining these error message constants in a location accessible to the
API module, or duplicating them as constants in TupleImpl to ensure consistency
and maintainability.
##########
modules/client/src/test/java/org/apache/ignite/client/ClientTupleTest.java:
##########
@@ -339,6 +350,59 @@ public void testToStringMatchesTupleImpl() {
clientTuple.toString().replace("ClientTuple ", ""));
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("primitiveAccessors")
+ @SuppressWarnings("ThrowableNotThrown")
+ void nullPointerWhenReadingNullAsPrimitive(
+ NativeType type,
+ BiConsumer<Tuple, Object> fieldAccessor
+ ) {
+ SchemaDescriptor schema = new SchemaDescriptor(
+ 1,
+ new Column[]{new Column("ID", INT32, false)},
+ new Column[]{new Column("VAL", type, true)}
+ );
+
+ ClientSchema clientSchema = new ClientSchema(
+ 1,
+ new ClientColumn[] {
+ new ClientColumn("ID", ColumnType.INT32, false, 0, -1,
-1, 0),
+ new ClientColumn("VAL", type.spec(), true, -1, 0, -1,
1),
+ },
+ marshallers
+ );
+
+ BinaryRow binaryRow = SchemaTestUtils.binaryRow(schema, 1, null);
+ Tuple row = new ClientTuple(clientSchema, TuplePart.KEY_AND_VAL, new
BinaryTupleReader(schema.length(), binaryRow.tupleSlice()));
+
+ assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, 1),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_ERROR_MESSAGE, 1)
+ );
+
+ assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, "VAL"),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_NAMED_ERROR_MESSAGE,
"VAL")
+ );
+
+ row.set("NEW", null);
+
+ assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, 2),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_ERROR_MESSAGE, 2)
+
Review Comment:
There is an inconsistent trailing blank line inside the assertThrows call.
For consistency with the rest of the codebase, this blank line should be
removed.
```suggestion
```
##########
modules/api/src/main/java/org/apache/ignite/table/TupleImpl.java:
##########
@@ -437,4 +437,26 @@ public String toString() {
return b.toString();
}
+
+ private <T> T valueNotNull(int columnIndex) {
+ T value = value(columnIndex);
+
+ if (value == null) {
+ throw new NullPointerException("The value of field at index " +
columnIndex
+ + " is null and cannot be converted to a primitive data
type.");
+ }
+
+ return value;
+ }
+
+ private <T> T valueNotNull(String columnName) {
+ T value = value(columnName);
+
+ if (value == null) {
+ throw new NullPointerException("The value of field '" + columnName
+ + "' is null and cannot be converted to a primitive data
type.");
+ }
Review Comment:
The error messages in TupleImpl are hardcoded and inconsistent with the
error message constants defined in IgniteUtils.NULL_TO_PRIMITIVE_ERROR_MESSAGE
and IgniteUtils.NULL_TO_PRIMITIVE_NAMED_ERROR_MESSAGE. While TupleImpl is in
the public API module and cannot depend on internal modules, the error messages
should be consistent across all implementations for a better user experience.
Consider defining these error message constants in a location accessible to the
API module, or duplicating them as constants in TupleImpl to ensure consistency
and maintainability.
##########
modules/table/src/test/java/org/apache/ignite/internal/table/MutableRowTupleAdapterTest.java:
##########
@@ -495,6 +508,78 @@ public void testFullSchemaHasAllTypes() {
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("primitiveAccessors")
+ @SuppressWarnings("ThrowableNotThrown")
+ void nullPointerWhenReadingNullAsPrimitive(
+ NativeType type,
+ BiConsumer<Tuple, Object> fieldAccessor
+ ) {
+ SchemaDescriptor schema = new SchemaDescriptor(
+ 1,
+ new Column[]{new Column("KEY", INT32, false)},
+ new Column[]{new Column("VAL", type, true)}
+ );
+
+ SchemaDescriptor schema2 = applyAddingValueColumn(schema, 1, new
Column("NEW", type, true));
+
+ Tuple tuple = Tuple.create().set("KEY", 1).set("VAL", null);
+ TupleMarshaller marshaller =
KeyValueTestUtils.createMarshaller(schema);
+
+ Row binRow = marshaller.marshal(tuple);
+ Tuple row = TableRow.tuple(binRow);
+
+ IgniteTestUtils.assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, 1),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_ERROR_MESSAGE, 1)
+ );
+
+ IgniteTestUtils.assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, "VAL"),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_NAMED_ERROR_MESSAGE,
"VAL")
+
+ );
Review Comment:
There is an inconsistent trailing blank line inside the assertThrows call.
For consistency with the rest of the codebase, this blank line should be
removed.
##########
modules/table/src/test/java/org/apache/ignite/internal/table/MutableRowTupleAdapterTest.java:
##########
@@ -495,6 +508,78 @@ public void testFullSchemaHasAllTypes() {
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("primitiveAccessors")
+ @SuppressWarnings("ThrowableNotThrown")
+ void nullPointerWhenReadingNullAsPrimitive(
+ NativeType type,
+ BiConsumer<Tuple, Object> fieldAccessor
+ ) {
+ SchemaDescriptor schema = new SchemaDescriptor(
+ 1,
+ new Column[]{new Column("KEY", INT32, false)},
+ new Column[]{new Column("VAL", type, true)}
+ );
+
+ SchemaDescriptor schema2 = applyAddingValueColumn(schema, 1, new
Column("NEW", type, true));
+
+ Tuple tuple = Tuple.create().set("KEY", 1).set("VAL", null);
+ TupleMarshaller marshaller =
KeyValueTestUtils.createMarshaller(schema);
+
+ Row binRow = marshaller.marshal(tuple);
+ Tuple row = TableRow.tuple(binRow);
+
+ IgniteTestUtils.assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, 1),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_ERROR_MESSAGE, 1)
+ );
+
+ IgniteTestUtils.assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, "VAL"),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_NAMED_ERROR_MESSAGE,
"VAL")
+
+ );
+
+ // Modify row.
+ row.set("NEW", null);
+
+ IgniteTestUtils.assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, 2),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_ERROR_MESSAGE, 2)
+
+ );
Review Comment:
There is an inconsistent trailing blank line inside the assertThrows call.
For consistency with the rest of the codebase, this blank line should be
removed.
##########
modules/table/src/test/java/org/apache/ignite/internal/table/MutableRowTupleAdapterTest.java:
##########
@@ -495,6 +508,78 @@ public void testFullSchemaHasAllTypes() {
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("primitiveAccessors")
+ @SuppressWarnings("ThrowableNotThrown")
+ void nullPointerWhenReadingNullAsPrimitive(
+ NativeType type,
+ BiConsumer<Tuple, Object> fieldAccessor
+ ) {
+ SchemaDescriptor schema = new SchemaDescriptor(
+ 1,
+ new Column[]{new Column("KEY", INT32, false)},
+ new Column[]{new Column("VAL", type, true)}
+ );
+
+ SchemaDescriptor schema2 = applyAddingValueColumn(schema, 1, new
Column("NEW", type, true));
+
+ Tuple tuple = Tuple.create().set("KEY", 1).set("VAL", null);
+ TupleMarshaller marshaller =
KeyValueTestUtils.createMarshaller(schema);
+
+ Row binRow = marshaller.marshal(tuple);
+ Tuple row = TableRow.tuple(binRow);
+
+ IgniteTestUtils.assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, 1),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_ERROR_MESSAGE, 1)
+ );
+
+ IgniteTestUtils.assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, "VAL"),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_NAMED_ERROR_MESSAGE,
"VAL")
+
+ );
+
+ // Modify row.
+ row.set("NEW", null);
+
+ IgniteTestUtils.assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, 2),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_ERROR_MESSAGE, 2)
+
+ );
+
+ IgniteTestUtils.assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(row, "NEW"),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_NAMED_ERROR_MESSAGE,
"NEW")
+ );
+
+ // Upgrade original row
+ var schemaRegistry = new SchemaRegistryImpl(
+ v -> v == 1 ? schema : schema2,
+ schema2
+ );
+
+ Tuple upgradedRow = TableRow.tuple(schemaRegistry.resolve(binRow,
schema2));
+
+ IgniteTestUtils.assertThrows(
+ NullPointerException.class,
+ () -> fieldAccessor.accept(upgradedRow, 2),
+
IgniteStringFormatter.format(IgniteUtils.NULL_TO_PRIMITIVE_ERROR_MESSAGE, 2)
+
+ );
Review Comment:
There is an inconsistent trailing blank line inside the assertThrows call.
For consistency with the rest of the codebase, this blank line should be
removed.
--
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]
