[GitHub] [ignite-3] korlov42 commented on a change in pull request #452: IGNITE-15785 POJO validation.

[GitBox <gi...@apache.org>]

korlov42 commented on a change in pull request #452:
URL: https://github.com/apache/ignite-3/pull/452#discussion_r751074276



##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/Column.java
##########
@@ -178,6 +181,15 @@ public Object defaultValue() {
         return defValSup.get();
     }
 
+    /**
+     * Get no default value flag: {@code true} if column hasn't default value, {@code false} - otherwise.
+     *
+     * @return {@code true} if column hasn't default value, {@code false} - otherwise.

Review comment:
       I don't quite understand what is actually "no default value" means. Could you please clarify? What is difference between an implicit NULL value supplier and explicit one?

##########
File path: modules/schema/src/test/java/org/apache/ignite/internal/schema/serializer/AbstractSerializerTest.java
##########
@@ -88,14 +88,14 @@ public void columnOrderSerializeTest() {
         AbstractSchemaSerializer assembler = SchemaSerializerImpl.INSTANCE;
 
         Column[] keyCols = {
-            new Column(0, "A", NativeTypes.UUID, false, () -> null),
-            new Column(1, "B", NativeTypes.INT64, false, () -> null),
-            new Column(2, "C", NativeTypes.INT8, false, () -> null),
+            new Column(0, "A", NativeTypes.UUID, false, null),

Review comment:
       Why did you change that?

##########
File path: modules/schema/src/test/java/org/apache/ignite/internal/schema/marshaller/RecordMarshallerTest.java
##########
@@ -424,9 +424,9 @@ public void ensureAllTypesChecked() {
                 new Column("primitiveByteCol", INT8, false, () -> (byte) 0x42),
                 new Column("primitiveShortCol", INT16, false, () -> (short) 0x4242),
                 new Column("primitiveIntCol", INT32, false, () -> 0x42424242),
-                new Column("primitiveFloatCol", FLOAT, false),
-                new Column("primitiveDoubleCol", DOUBLE, false),
-                
+                new Column("primitiveFloatCol", FLOAT, false, () -> 100.100),

Review comment:
       Why did you change that?

##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/Column.java
##########
@@ -96,16 +99,16 @@ public Column(
      * @param name        Column name.
      * @param type        An instance of column data type.
      * @param nullable    If {@code false}, null values will not be allowed for this column.
-     * @param defValSup   Default value supplier.
+     * @param defValSup   Default value supplier or {@code null} - if there is no default value supplier specified.
      */
     public Column(
             int columnOrder,
             String name,
             NativeType type,
             boolean nullable,
-            @NotNull Supplier<Object> defValSup
+            Supplier<Object> defValSup

Review comment:
       I'm not sure if removing annotations here is a good idea. There is already a constructor in which a similar parameter is required, so the same should be done here.

##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/marshaller/reflection/Marshaller.java
##########
@@ -215,6 +232,10 @@ public Object readObject(Row reader) throws MarshallerException {
         @Override
         public void writeObject(Object obj, RowAssembler writer)
                 throws MarshallerException {
+            if (readOnly) {

Review comment:
       Furthermore the current behaviour will be changed in case the user provide a null value supplier explicitly. 

##########
File path: modules/schema/src/test/java/org/apache/ignite/internal/schema/SchemaDescriptorTest.java
##########
@@ -63,15 +63,15 @@ public void columnIndexedAccess() {
     @Test
     public void columnOrder() {
         Column[] keyColumns = {
-                new Column(0, "columnA", NativeTypes.INT8, false, () -> null),
-                new Column(1, "columnB", NativeTypes.UUID, false, () -> null),
-                new Column(2, "columnC", NativeTypes.INT32, false, () -> null),
+                new Column(0, "columnA", NativeTypes.INT8, false, null),

Review comment:
       Why did you change that?

##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/marshaller/reflection/Marshaller.java
##########
@@ -215,6 +232,10 @@ public Object readObject(Row reader) throws MarshallerException {
         @Override
         public void writeObject(Object obj, RowAssembler writer)
                 throws MarshallerException {
+            if (readOnly) {

Review comment:
       Why do we need such a limitation? To me the previous behaviour is much better, because you got here an exception describing a root cause of the issue (setting null to a non-null field). Instead we now got here an abstract "Can't assemple row by object".
   
   BTW there is a typo in "assemble".




-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org