This is an automated email from the ASF dual-hosted git repository. amashenkov pushed a commit to branch ignite-13618 in repository https://gitbox.apache.org/repos/asf/ignite.git
commit 413126148a73bbd8b3eac7dc3f6bf5c5843d83fd Author: Andrew Mashenkov <andrey.mashen...@gmail.com> AuthorDate: Fri Nov 20 15:44:27 2020 +0300 Fix broken null fields serialization in JaninoSerilizer. Tests added. --- .../generator/FieldAccessExprGenerator.java | 206 ++++++++++++++++++++- .../generator/JaninoSerializerGenerator.java | 162 +--------------- .../schema/marshaller/JavaSerializerTest.java | 39 +--- .../marshaller/reflection/FieldAccessorTest.java | 98 ++++++++-- 4 files changed, 300 insertions(+), 205 deletions(-) diff --git a/modules/commons/src/main/java/org/apache/ignite/internal/schema/marshaller/generator/FieldAccessExprGenerator.java b/modules/commons/src/main/java/org/apache/ignite/internal/schema/marshaller/generator/FieldAccessExprGenerator.java index e596e13..9762167 100644 --- a/modules/commons/src/main/java/org/apache/ignite/internal/schema/marshaller/generator/FieldAccessExprGenerator.java +++ b/modules/commons/src/main/java/org/apache/ignite/internal/schema/marshaller/generator/FieldAccessExprGenerator.java @@ -17,12 +17,168 @@ package org.apache.ignite.internal.schema.marshaller.generator; +import org.apache.ignite.internal.schema.marshaller.BinaryMode; import org.jetbrains.annotations.Nullable; +import static org.apache.ignite.internal.schema.marshaller.generator.JaninoSerializerGenerator.LF; + /** * Object field access expression generators. */ class FieldAccessExprGenerator { + /** Append null expression. */ + private static final String WRITE_NULL_EXPR = "asm.appendNull();"; + + /** + * Created object field access expressions generator. + * + * @param mode Field access binary mode. + * @param colIdx Column absolute index in schema. + * @param offset Object field offset. + * @return Object field access expressions generator. + */ + static FieldAccessExprGenerator createAccessor(BinaryMode mode, int colIdx, long offset) { + switch (mode) { + case BYTE: + return new FieldAccessExprGenerator( + colIdx, + "Byte", + "tuple.byteValueBoxed", + "asm.appendByte", + offset); + + case P_BYTE: + return new FieldAccessExprGenerator( + colIdx, + "tuple.byteValue", + "asm.appendByte", + offset, + "IgniteUnsafeUtils.getByteField", + "IgniteUnsafeUtils.putByteField" + ); + + case SHORT: + return new FieldAccessExprGenerator( + colIdx, + "Short", + "tuple.shortValueBoxed", + "asm.appendShort", + offset); + + case P_SHORT: + return new FieldAccessExprGenerator( + colIdx, + "tuple.shortValue", + "asm.appendShort", + offset, + "IgniteUnsafeUtils.getShortField", + "IgniteUnsafeUtils.putShortField" + ); + + case INT: + return new FieldAccessExprGenerator( + colIdx, + "Integer", + "tuple.intValueBoxed", + "asm.appendInt", + offset); + + case P_INT: + return new FieldAccessExprGenerator( + colIdx, + "tuple.intValue", + "asm.appendInt", + offset, + "IgniteUnsafeUtils.getIntField", + "IgniteUnsafeUtils.putIntField" + ); + + case LONG: + return new FieldAccessExprGenerator( + colIdx, + "Long", + "tuple.longValueBoxed", + "asm.appendLong", + offset); + + case P_LONG: + return new FieldAccessExprGenerator( + colIdx, + "tuple.longValue", + "asm.appendLong", + offset, + "IgniteUnsafeUtils.getLongField", + "IgniteUnsafeUtils.putLongField" + ); + + case FLOAT: + return new FieldAccessExprGenerator( + colIdx, + "Float", + "tuple.floatValueBoxed", + "asm.appendFloat", + offset); + + case P_FLOAT: + return new FieldAccessExprGenerator( + colIdx, + "tuple.floatValue", + "asm.appendFloat", + offset, + "IgniteUnsafeUtils.getFloatField", + "IgniteUnsafeUtils.putFloatField" + ); + + case DOUBLE: + return new FieldAccessExprGenerator( + colIdx, + "Double", + "tuple.doubleValueBoxed", + "asm.appendDouble", + offset); + + case P_DOUBLE: + return new FieldAccessExprGenerator( + colIdx, + "tuple.doubleValue", + "asm.appendDouble", + offset, + "IgniteUnsafeUtils.getDoubleField", + "IgniteUnsafeUtils.putDoubleField" + ); + + case UUID: + return new FieldAccessExprGenerator( + colIdx, + "UUID", + "tuple.uuidValue", "asm.appendUuid", + offset); + + case BITSET: + return new FieldAccessExprGenerator( + colIdx, + "BitSet", + "tuple.bitmaskValue", "asm.appendBitmask", + offset); + + case STRING: + return new FieldAccessExprGenerator( + colIdx, + "String", + "tuple.stringValue", "asm.appendString", + offset); + + case BYTE_ARR: + return new FieldAccessExprGenerator( + colIdx, + "byte[]", + "tuple.bytesValue", "asm.appendBytes", + offset); + default: + throw new IllegalStateException("Unsupportd binary mode"); + } + } + /** Object field offset or {@code -1} for identity accessor. */ private final long offset; @@ -30,7 +186,7 @@ class FieldAccessExprGenerator { private final int colIdx; /** Class cast expression. */ - private final String castClassExpr; + private final String classExpr; /** Write column value expression. */ private final String writeColMethod; @@ -53,7 +209,7 @@ class FieldAccessExprGenerator { * @param writeColMethod Write column value expression. * @param offset Field offset or {@code -1} for identity accessor. */ - public FieldAccessExprGenerator( + private FieldAccessExprGenerator( int colIdx, String castClassExpr, String readColMethod, @@ -107,7 +263,7 @@ class FieldAccessExprGenerator { ) { this.offset = offset; this.colIdx = colIdx; - this.castClassExpr = castClassExpr; + this.classExpr = castClassExpr; this.putFieldMethod = putFieldMethod; this.getFieldMethod = getFieldMethod; this.writeColMethod = writeColMethod; @@ -115,10 +271,24 @@ class FieldAccessExprGenerator { } /** + * @return {@code true} if it is primitive typed field accessor, {@code false} otherwise. + */ + private boolean isPrimitive() { + return classExpr == null; + } + + /** + * @return {@code true} if is identity accessor, {@code false} otherwise. + */ + private boolean isIdentityAccessor() { + return offset == -1; + } + + /** * @return Object field value access expression or object value expression for simple types. */ public String getFieldExpr() { - if (offset == -1) + if (isIdentityAccessor()) return "obj"; // Identity accessor. return getFieldMethod + "(obj, " + offset + ')'; @@ -133,7 +303,7 @@ class FieldAccessExprGenerator { */ public final void appendPutFieldExpr(StringBuilder sb, String valueExpression, String indent) { sb.append(indent).append(putFieldMethod).append("(obj, ").append(offset).append(", ").append(valueExpression).append(')'); - sb.append(";" + JaninoSerializerGenerator.LF); + sb.append(";" + LF); } /** @@ -144,12 +314,30 @@ class FieldAccessExprGenerator { * @param indent Line indentation. */ public final void appendWriteColumnExpr(StringBuilder sb, String valueExpr, String indent) { - sb.append(indent).append(writeColMethod).append('('); + if (isPrimitive() || isIdentityAccessor()) { + // Translate to: + // asm.appendX((T) %value%); + // or for primitive value: + // asm.appendX(%value%); + sb.append(indent).append(writeColMethod).append('('); + + if (classExpr != null) + sb.append("(").append(classExpr).append(")"); + + sb.append(valueExpr).append(");" + LF); + + return; + } + + assert classExpr != null; - if (castClassExpr != null) - sb.append("(").append(castClassExpr).append(")"); + // Translate to: + // { T fVal = (T)%value%; + // if (fVal == null) asm.appendNull() else asm.appendX(fVal); } + sb.append(indent).append("{ ").append(classExpr).append(" fVal = (").append(classExpr).append(')').append(valueExpr).append(";" + LF); + sb.append(indent).append("if (fVal == null) " + WRITE_NULL_EXPR + LF); + sb.append(indent).append("else ").append(writeColMethod).append("(fVal); }" + LF); - sb.append(valueExpr).append(");" + JaninoSerializerGenerator.LF); } /** diff --git a/modules/commons/src/main/java/org/apache/ignite/internal/schema/marshaller/generator/JaninoSerializerGenerator.java b/modules/commons/src/main/java/org/apache/ignite/internal/schema/marshaller/generator/JaninoSerializerGenerator.java index 8d88b2f..21a3201 100644 --- a/modules/commons/src/main/java/org/apache/ignite/internal/schema/marshaller/generator/JaninoSerializerGenerator.java +++ b/modules/commons/src/main/java/org/apache/ignite/internal/schema/marshaller/generator/JaninoSerializerGenerator.java @@ -44,7 +44,7 @@ public class JaninoSerializerGenerator implements SerializerFactory { public static final int INITIAL_BUFFER_SIZE = 8 * 1024; /** Debug flag. */ - private static final boolean enabledDebug = false; + private static final boolean enabledDebug = true; /** {@inheritDoc} */ @Override public Serializer create( @@ -60,9 +60,13 @@ public class JaninoSerializerGenerator implements SerializerFactory { //TODO: pass code to logger on trace level. - if (enabledDebug) + if (enabledDebug) { ce.setDebuggingInformation(true, true, true); + //TODO: dump code to log. + System.out.println(code); + } + try { // Compile and load class. ce.setParentClassLoader(getClass().getClassLoader()); ce.cook(code); @@ -168,14 +172,14 @@ public class JaninoSerializerGenerator implements SerializerFactory { BinaryMode mode = MarshallerUtil.mode(aClass); if (mode != null) - return new IdentityObjectMarshallerExprGenerator(createAccessor(mode, firstColIdx, -1L)); + return new IdentityObjectMarshallerExprGenerator(FieldAccessExprGenerator.createAccessor(mode, firstColIdx, -1L)); FieldAccessExprGenerator[] accessors = new FieldAccessExprGenerator[columns.length()]; try { for (int i = 0; i < columns.length(); i++) { final Field field = aClass.getDeclaredField(columns.column(i).name()); - accessors[i] = createAccessor( + accessors[i] = FieldAccessExprGenerator.createAccessor( MarshallerUtil.mode(field.getType()), firstColIdx + i /* schma absolute index. */, IgniteUnsafeUtils.objectFieldOffset(field)); @@ -189,156 +193,6 @@ public class JaninoSerializerGenerator implements SerializerFactory { } /** - * Created object field access expressions generator. - * - * @param mode Field access binary mode. - * @param colIdx Column absolute index in schema. - * @param offset Object field offset. - * @return Object field access expressions generator. - */ - private FieldAccessExprGenerator createAccessor(BinaryMode mode, int colIdx, long offset) { - switch (mode) { - case BYTE: - return new FieldAccessExprGenerator( - colIdx, - "Byte", - "tuple.byteValueBoxed", - "asm.appendByte", - offset); - - case P_BYTE: - return new FieldAccessExprGenerator( - colIdx, - "tuple.byteValue", - "asm.appendByte", - offset, - "IgniteUnsafeUtils.getByteField", - "IgniteUnsafeUtils.putByteField" - ); - - case SHORT: - return new FieldAccessExprGenerator( - colIdx, - "Short", - "tuple.shortValueBoxed", - "asm.appendShort", - offset); - - case P_SHORT: - return new FieldAccessExprGenerator( - colIdx, - "tuple.shortValue", - "asm.appendShort", - offset, - "IgniteUnsafeUtils.getShortField", - "IgniteUnsafeUtils.putShortField" - ); - - case INT: - return new FieldAccessExprGenerator( - colIdx, - "Integer", - "tuple.intValueBoxed", - "asm.appendInt", - offset); - - case P_INT: - return new FieldAccessExprGenerator( - colIdx, - "tuple.intValue", - "asm.appendInt", - offset, - "IgniteUnsafeUtils.getIntField", - "IgniteUnsafeUtils.putIntField" - ); - - case LONG: - return new FieldAccessExprGenerator( - colIdx, - "Long", - "tuple.longValueBoxed", - "asm.appendLong", - offset); - - case P_LONG: - return new FieldAccessExprGenerator( - colIdx, - "tuple.longValue", - "asm.appendLong", - offset, - "IgniteUnsafeUtils.getLongField", - "IgniteUnsafeUtils.putLongField" - ); - - case FLOAT: - return new FieldAccessExprGenerator( - colIdx, - "Float", - "tuple.floatValueBoxed", - "asm.appendFloat", - offset); - - case P_FLOAT: - return new FieldAccessExprGenerator( - colIdx, - "tuple.floatValue", - "asm.appendFloat", - offset, - "IgniteUnsafeUtils.getFloatField", - "IgniteUnsafeUtils.putFloatField" - ); - - case DOUBLE: - return new FieldAccessExprGenerator( - colIdx, - "Double", - "tuple.doubleValueBoxed", - "asm.appendDouble", - offset); - - case P_DOUBLE: - return new FieldAccessExprGenerator( - colIdx, - "tuple.doubleValue", - "asm.appendDouble", - offset, - "IgniteUnsafeUtils.getDoubleField", - "IgniteUnsafeUtils.putDoubleField" - ); - - case UUID: - return new FieldAccessExprGenerator( - colIdx, - "UUID", - "tuple.uuidValue", "asm.appendUuid", - offset); - - case BITSET: - return new FieldAccessExprGenerator( - colIdx, - "BitSet", - "tuple.bitmaskValue", "asm.appendBitmask", - offset); - - case STRING: - return new FieldAccessExprGenerator( - colIdx, - "String", - "tuple.stringValue", "asm.appendString", - offset); - - case BYTE_ARR: - return new FieldAccessExprGenerator( - colIdx, - "byte[]", - "tuple.bytesValue", "asm.appendBytes", - offset); - default: - throw new IllegalStateException("Unsupportd binary mode"); - } - } - - /** * Appends {@link Serializer#serialize(Object, Object)} method code. * * @param sb String buffer to append to. diff --git a/modules/commons/src/test/java/org/apache/ignite/internal/schema/marshaller/JavaSerializerTest.java b/modules/commons/src/test/java/org/apache/ignite/internal/schema/marshaller/JavaSerializerTest.java index 8899b8c..babc7cb 100644 --- a/modules/commons/src/test/java/org/apache/ignite/internal/schema/marshaller/JavaSerializerTest.java +++ b/modules/commons/src/test/java/org/apache/ignite/internal/schema/marshaller/JavaSerializerTest.java @@ -90,7 +90,8 @@ public class JavaSerializerTest { NativeType[] types = new NativeType[] {BYTE, SHORT, INTEGER, LONG, FLOAT, DOUBLE, UUID, STRING, BYTES, Bitmask.of(5)}; return serializerFactoryProvider().stream().map(factory -> - dynamicContainer(factory.getClass().getSimpleName(), + dynamicContainer( + factory.getClass().getSimpleName(), Stream.concat( // Test pure types. Stream.of(types).map(type -> @@ -110,34 +111,6 @@ public class JavaSerializerTest { } /** - * - */ - @ParameterizedTest - @MethodSource("serializerFactoryProvider") - public void testBasicTypes(SerializerFactory factory) throws SerializationException { - // Fixed types: - checkBasicType(factory, BYTE, BYTE); - checkBasicType(factory, SHORT, SHORT); - checkBasicType(factory, INTEGER, INTEGER); - checkBasicType(factory, LONG, LONG); - checkBasicType(factory, FLOAT, FLOAT); - checkBasicType(factory, DOUBLE, DOUBLE); - checkBasicType(factory, UUID, UUID); - checkBasicType(factory, Bitmask.of(4), Bitmask.of(5)); - - // Varlen types: - checkBasicType(factory, BYTES, BYTES); - checkBasicType(factory, STRING, STRING); - - // Mixed: - checkBasicType(factory, LONG, INTEGER); - checkBasicType(factory, FLOAT, DOUBLE); - checkBasicType(factory, INTEGER, BYTES); - checkBasicType(factory, STRING, LONG); - checkBasicType(factory, Bitmask.of(9), BYTES); - } - - /** * @throws SerializationException If serialization failed. */ @ParameterizedTest @@ -155,12 +128,14 @@ public class JavaSerializerTest { new Column("shortCol", SHORT, true), new Column("intCol", INTEGER, true), new Column("longCol", LONG, true), + new Column("nullLongCol", LONG, true), new Column("floatCol", FLOAT, true), new Column("doubleCol", DOUBLE, true), new Column("uuidCol", UUID, true), new Column("bitmaskCol", Bitmask.of(42), true), new Column("stringCol", STRING, true), + new Column("nullBytesCol", BYTES, true), new Column("bytesCol", BYTES, true), }; @@ -393,6 +368,7 @@ public class JavaSerializerTest { private Short shortCol; private Integer intCol; private Long longCol; + private Long nullLongCol; private Float floatCol; private Double doubleCol; @@ -400,6 +376,7 @@ public class JavaSerializerTest { private BitSet bitmaskCol; private String stringCol; private byte[] bytesCol; + private byte[] nullBytesCol; /** {@inheritDoc} */ @Override public boolean equals(Object o) { @@ -421,12 +398,14 @@ public class JavaSerializerTest { Objects.equals(shortCol, object.shortCol) && Objects.equals(intCol, object.intCol) && Objects.equals(longCol, object.longCol) && + Objects.equals(nullLongCol, object.nullLongCol) && Objects.equals(floatCol, object.floatCol) && Objects.equals(doubleCol, object.doubleCol) && Objects.equals(uuidCol, object.uuidCol) && Objects.equals(bitmaskCol, object.bitmaskCol) && Objects.equals(stringCol, object.stringCol) && - Arrays.equals(bytesCol, object.bytesCol); + Arrays.equals(bytesCol, object.bytesCol) && + Arrays.equals(nullBytesCol, object.nullBytesCol); } /** {@inheritDoc} */ diff --git a/modules/commons/src/test/java/org/apache/ignite/internal/schema/marshaller/reflection/FieldAccessorTest.java b/modules/commons/src/test/java/org/apache/ignite/internal/schema/marshaller/reflection/FieldAccessorTest.java index 0d4c4a5..8e864d6 100644 --- a/modules/commons/src/test/java/org/apache/ignite/internal/schema/marshaller/reflection/FieldAccessorTest.java +++ b/modules/commons/src/test/java/org/apache/ignite/internal/schema/marshaller/reflection/FieldAccessorTest.java @@ -82,17 +82,17 @@ public class FieldAccessorTest { new Column("pFloatCol", FLOAT, false), new Column("pDoubleCol", DOUBLE, false), - new Column("byteCol", BYTE, true), - new Column("shortCol", SHORT, true), - new Column("intCol", INTEGER, true), - new Column("longCol", LONG, true), - new Column("floatCol", FLOAT, true), - new Column("doubleCol", DOUBLE, true), - - new Column("uuidCol", UUID, true), - new Column("bitmaskCol", Bitmask.of(9), true), - new Column("stringCol", STRING, true), - new Column("bytesCol", BYTES, true), + new Column("byteCol", BYTE, false), + new Column("shortCol", SHORT, false), + new Column("intCol", INTEGER, false), + new Column("longCol", LONG, false), + new Column("floatCol", FLOAT, false), + new Column("doubleCol", DOUBLE, false), + + new Column("uuidCol", UUID, false), + new Column("bitmaskCol", Bitmask.of(9), false), + new Column("stringCol", STRING, false), + new Column("bytesCol", BYTES, false), }; final Pair<TupleAssembler, Tuple> mocks = createMocks(); @@ -140,6 +140,49 @@ public class FieldAccessorTest { * @throws Exception If failed. */ @Test + public void testNullableFieldsAccessor() throws Exception { + Column[] cols = new Column[] { + new Column("intCol", INTEGER, true), + new Column("longCol", LONG, true), + + new Column("stringCol", STRING, true), + new Column("bytesCol", BYTES, true), + }; + + final Pair<TupleAssembler, Tuple> mocks = createMocks(); + + final TupleAssembler tupleAssembler = mocks.getFirst(); + final Tuple tuple = mocks.getSecond(); + + final TestSimpleObject obj = new TestSimpleObject(); + obj.longCol = rnd.nextLong(); + obj.stringCol = TestUtils.randomString(rnd, 255); + + for (int i = 0; i < cols.length; i++) { + UnsafeFieldAccessor accessor = UnsafeFieldAccessor.create(TestSimpleObject.class, cols[i], i); + + accessor.write(obj, tupleAssembler); + } + + final TestSimpleObject restoredObj = new TestSimpleObject(); + + for (int i = 0; i < cols.length; i++) { + UnsafeFieldAccessor accessor = UnsafeFieldAccessor.create(TestSimpleObject.class, cols[i], i); + + accessor.read(restoredObj, tuple); + } + + assertEquals(obj.intCol, restoredObj.intCol); + assertEquals(obj.longCol, restoredObj.longCol); + + assertEquals(obj.stringCol, restoredObj.stringCol); + assertArrayEquals(obj.bytesCol, restoredObj.bytesCol); + } + + /** + * @throws Exception If failed. + */ + @Test public void testIdentityAccessor() throws Exception { final UnsafeFieldAccessor accessor = UnsafeFieldAccessor.createIdentityAccessor( new Column("col0", STRING, true), @@ -188,7 +231,10 @@ public class FieldAccessorTest { final Answer<Void> asmAnswer = new Answer<Void>() { @Override public Void answer(InvocationOnMock invocation) { - vals.add(invocation.getArguments()[0]); + if ("appendNull".equals(invocation.getMethod().getName())) + vals.add(null); + else + vals.add(invocation.getArguments()[0]); return null; } @@ -319,4 +365,32 @@ public class FieldAccessorTest { return 73; } } + + /** + * Test object. + */ + private static class TestSimpleObject { + Long longCol; + Integer intCol; + byte[] bytesCol; + String stringCol; + + /** {@inheritDoc} */ + @Override public boolean equals(Object o) { + if (this == o) + return true; + if (o == null || getClass() != o.getClass()) + return false; + TestSimpleObject object = (TestSimpleObject)o; + return Objects.equals(longCol, object.longCol) && + Objects.equals(intCol, object.intCol) && + Arrays.equals(bytesCol, object.bytesCol) && + Objects.equals(stringCol, object.stringCol); + } + + /** {@inheritDoc} */ + @Override public int hashCode() { + return 42; + } + } }