[SPARK-22000][SQL][FOLLOWUP] Add comment and simple cleanup to DeserializerBuildHelper #23916
Conversation
| */ | ||
| def deserializerForWithNullSafety( | ||
| expr: Expression, | ||
| dataType: DataType, |
There was a problem hiding this comment.
dataType is useless here.
| nullable = nullable, | ||
| newTypePath, | ||
| (expr, typePath) => { | ||
| // For tuples, we based grab the inner fields by ordinal instead of name. |
There was a problem hiding this comment.
This location should be more proper for this comment.
| * @param funcForCreatingDeserializer Given input expression and typed path, this function | ||
| * returns deserializer expression. | ||
| */ | ||
| def deserializerForWithNullSafety( |
There was a problem hiding this comment.
we can remove it, according to #23908 (comment)
There was a problem hiding this comment.
Remove deserializerForWithNullSafety? So we call deserializerForWithNullSafetyAndUpcast for all cases?
There was a problem hiding this comment.
have you read the comment? It's the same as expressionWithNullSafety
There was a problem hiding this comment.
yes, but looks like to remove expressionWithNullSafety and incorporate it into deserializerForWithNullSafety?
There was a problem hiding this comment.
oh, i see. I don't see the all change around the change to deserializerForWithNullSafet there...since it is not shown if not clicking changes tab.
There was a problem hiding this comment.
I think expressionWithNullSafety is more general naming so might be preferred one, but deserializerForWithNullSafety is also a good name cause we have relevant method deserializerForWithNullSafetyAndUpcast.
So that's a matter of preference and either can be removed. Which method would we prefer to keep?
There was a problem hiding this comment.
Moving my comment to #23908 since it will keep updated.
|
#23908 continues to make a lot change to |
|
Test build #102851 has finished for PR 23916 at commit
|
5 participants
What changes were proposed in this pull request?
#23854 adds some help methods used for creating deserializers. This is a followup for it to add comments and do simple cleanup.
How was this patch tested?
Existing tests.