Fixed a bug when generating specific FastSerializer with Union of Stringable type #71
Conversation
...enerated/serialization/AVRO_1_4/StringableRecord_SpecificSerializer_6174384286732341990.java
Outdated
Show resolved
Hide resolved
...enerated/serialization/AVRO_1_8/StringableRecord_SpecificSerializer_6174384286732341990.java
Outdated
Show resolved
Hide resolved
.../AVRO_1_7/StringableRecord_SpecificDeserializer_6174384286732341990_6174384286732341990.java
Outdated
Show resolved
Hide resolved
|
|
||
| // when | ||
| StringableRecord afterDecoding = null; | ||
| if (whetherUseFastDeserializer) { | ||
| afterDecoding = | ||
| readWithFastAvro(StringableRecord.SCHEMA$, StringableRecord.SCHEMA$, specificDataAsDecoder(record), true); | ||
| readWithFastAvro(StringableRecord.SCHEMA$, StringableRecord.SCHEMA$, writeWithFastAvro(record, StringableRecord.SCHEMA$, true), true); |
There was a problem hiding this comment.
It would be nice if the test change was in a separate commit, since we cannot see what changed in the generated code... We only see that there is new generated code, but not what the previous state was.
There was a problem hiding this comment.
If you look at the diff side-by-side, you should be able to figure out what changes with the test change.
github treats those generated files as moved, so the diff is clear to me.
Let us know if it doesn't work for you.
There was a problem hiding this comment.
So are you saying that the test change resulted in zero change to the generated code? I thought the test added a new field to the schema, which should change the generated code. Otherwise, how can we see how the generated code is different, if the change touches only a part which was previously not getting tested/generated?
There was a problem hiding this comment.
I got your point.
The avro schema change did result in changes in the generated code for both serializer and de-serializer, and the change to the schema is to verify the fix.
Without the latest code gen logic, the generated fast serializer couldn't compile since it would generate something like this:
String stringUnion0 = ((String) data.get(9));
if (stringUnion0 == null) {
(encoder).writeIndex(0);
(encoder).writeNull();
} else {
(encoder).writeIndex(1);
if (stringUnion0 instanceof Utf8) {
(encoder).writeString(((Utf8) stringUnion0));
} else {
(encoder).writeString(stringUnion0 .toString());
}
}
If we want to compare the difference before/after code gen change with the change of test schema, we need to checkin the invalid java file, it could be fine since it is temporary, but I am wondering whether it is mandatory for reviewing.
As long as the new part of the generated code is reasonable for the new change to the test schema, it should be fine, right?
…ingable type Along with the bug fix, this code change always optimized the if/else condition for union/enum handling, and also removed the unnecessary type check for String field handling.
| } else { | ||
| if (unionIndex1 == 1) { | ||
| int enumIndex3 = (decoder.readEnum()); | ||
| org.apache.avro.generic.GenericData.EnumSymbol enumValue3 = null; | ||
| if (enumIndex3 == 0) { | ||
| enumValue3 = new org.apache.avro.generic.GenericData.EnumSymbol(testEnum0 .getEnumSymbols().get(1)); | ||
| } else { | ||
| if (enumIndex3 == 1) { | ||
| enumValue3 = new org.apache.avro.generic.GenericData.EnumSymbol(testEnum0 .getEnumSymbols().get(0)); | ||
| < 8000 /td> | } else { | |
| if (enumIndex3 == 2) { | ||
| enumValue3 = new org.apache.avro.generic.GenericData.EnumSymbol(testEnum0 .getEnumSymbols().get(4)); | ||
| } else { | ||
| if (enumIndex3 == 3) { | ||
| enumValue3 = new org.apache.avro.generic.GenericData.EnumSymbol(testEnum0 .getEnumSymbols().get(2)); | ||
| } else { | ||
| if (enumIndex3 == 4) { | ||
| enumValue3 = new org.apache.avro.generic.GenericData.EnumSymbol(testEnum0 .getEnumSymbols().get(3)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| testEnumUnionArray1 .add(enumValue3); |
There was a problem hiding this comment.
The new structure is very nested xD ... it doesn't look super clean but it's ok, performance-wise it should be identical to else if ()
|
Thanks @FelixGV for the review! |
@FelixGV @radai-rosenblatt @volauvent