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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
