Added specific record tests for primitive arrays. #81
Conversation
Codecov Report
@@ Coverage Diff @@
## master #81 +/- ##
=========================================
Coverage 53.52% 53.52%
Complexity 275 275
=========================================
Files 39 39
Lines 1659 1659
Branches 206 206
=========================================
Hits 888 888
Misses 689 689
Partials 82 82 Continue to review full report at Codecov.
|
| @@ -87,7 +87,7 @@ public StringableRecord_SpecificDeserializer_6174384286732341990_617438428673234 | |||
| chunkLen0 = (decoder.arrayNext()); | |||
| } while (chunkLen0 > 0); | |||
| } else { | |||
| urlArray0 = Collections.emptyList(); | |||
| urlArray0 = new ArrayList<Utf8>(); | |||
There was a problem hiding this comment.
nit - initial size 1?
There was a problem hiding this comment.
why not 0 since it's always an empty ArrayList? :)
There was a problem hiding this comment.
I don't really like the structure of that generated code, and I was meaning to tweak it anyway at some point... so might as well do it now.
| @@ -144,6 +146,9 @@ | |||
| } catch (JClassAlreadyExistsException e) { | |||
| throw new FastDeserializerGeneratorException("Class: " + className + " already exists"); | |||
| } catch (Exception e) { | |||
| LOGGER.error("Failed to generate a deserializer."); | |||
There was a problem hiding this comment.
better to have this as a single line (maybe with \n) because in a busy system other prints can get injected in the middle on the log file
There was a problem hiding this comment.
also - use {} and dont call toString() (less important for error, but still)
There was a problem hiding this comment.
We do not need to log here since the FastDeserializerGeneratorException would be catched in upper layer buildGeneric/SpecificDeserializer and schemas would be logged?
There was a problem hiding this comment.
Hmm... I think the thing is that we have test code that invokes this function directly, and therefore does not benefit from the upstream logging that happens in the main code. I will remove those extra logs then.
| @@ -662,10 +667,8 @@ private void processArray(JVar arraySchemaVar, final String name, final Schema a | |||
| } | |||
| doLoop.body().assign(chunkLen, JExpr.direct(DECODER + ".arrayNext()")); | |||
| }, else1 -> { | |||
| if (useGenericTypes) { | |||
| if (finalAction.getShouldRead()) { | |||
| else1.assign(arrayVar, finalNewArrayExp); | |||
There was a problem hiding this comment.
this generates an arraylist of size 10, i think it should make it smaller (since its always empty here)
| @@ -87,7 +87,7 @@ public StringableRecord_SpecificDeserializer_6174384286732341990_617438428673234 | |||
| chunkLen0 = (decoder.arrayNext()); | |||
| } while (chunkLen0 > 0); | |||
| } else { | |||
| urlArray0 = Collections.emptyList(); | |||
| urlArray0 = new ArrayList<Utf8>(); | |||
There was a problem hiding this comment.
why not 0 since it's always an empty ArrayList? :)
| @@ -144,6 +146,9 @@ | |||
| } catch (JClassAlreadyExistsException e) { | |||
| throw new FastDeserializerGeneratorException("Class: " + className + " already exists"); | |||
| } catch (Exception e) { | |||
| LOGGER.error("Failed to generate a deserializer."); | |||
There was a problem hiding this comment.
We do not need to log here since the FastDeserializerGeneratorException would be catched in upper layer buildGeneric/SpecificDeserializer and schemas would be logged?
| @@ -350,6 +350,54 @@ | |||
| } | |||
| }, | |||
| "default": {"test": [null]} | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
No code-gen results for these new fields in this PR?
There was a problem hiding this comment.
No, we currently don't add this to the codegen directory. Only some of the tests are put in there. Perhaps those should be added...
Addressed bugs related to missing constructors in the PrimitiveArrayList and its child classes. Also refactored those classes slightly to reduce boilerplate. Altered the meta code: - List construction does not rely on Collections.emptyList() since that is incompatible with the primitive list classes. - Fixed a case where list instantiation wasn't guarded by getShouldRead(), when schema evolution gets rid of a list field. This would previously cause a NPE during code gen. - Refactored the list handling code so that list instance reuse occurs even though the particular payload being deserialized has zero elements.
79ff2d5 to
22efffc
Compare
|
|
||
| ifCodeGen(parentBody, chunkLengthGreaterThanZero, then1 -> { |
There was a problem hiding this comment.
FYI: this looks like a big change but it's mostly just a move and an indentation change. The list instantiation (and potential reuse) now happens first, and the if chunk length > 0 check as well as its else block is now removed. Then most of the remaining code is just an indentation change...
Addressed bugs related to missing constructors in the PrimitiveArrayList
and its child classes. Also refactored those classes slightly to reduce
boilerplate.
Altered the meta code:
is incompatible with the primitive list classes.
gets rid of a list field. This would previously cause a NPE during
code gen.