Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,25 +44,38 @@ object DeserializerBuildHelper {
upCastToExpectedType(newPath, dataType, walkedTypePath)
}

/**
* Returns an expression that can be used to deserialize input expression.
*
* @param expr The input expression that can be used to extract serialized value.
* @param nullable Whether deserialized expression evalutes to null value.
* @param walkedTypePath The paths from top to bottom to access current field when deserializing.
* @param funcForCreatingDeserializer Given input expression and typed path, this function
* returns deserializer expression.
*/
def deserializerForWithNullSafety(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can remove it, according to #23908 (comment)

Copy link
Member Author

@viirya viirya Feb 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove deserializerForWithNullSafety? So we call deserializerForWithNullSafetyAndUpcast for all cases?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you read the comment? It's the same as expressionWithNullSafety

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but looks like to remove expressionWithNullSafety and incorporate it into deserializerForWithNullSafety?

Copy link
Member Author

@viirya viirya Feb 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving my comment to #23908 since it will keep updated.

expr: Expression,
dataType: DataType,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dataType is useless here.

nullable: Boolean,
walkedTypePath: Seq[String],
funcForCreatingNewExpr: (Expression, Seq[String]) => Expression): Expression = {
val newExpr = funcForCreatingNewExpr(expr, walkedTypePath)
funcForCreatingDeserializer: (Expression, Seq[String]) => Expression): Expression = {
val newExpr = funcForCreatingDeserializer(expr, walkedTypePath)
expressionWithNullSafety(newExpr, nullable, walkedTypePath)
}

/**
* This returns deserializer expression as `deserializerForWithNullSafety` does. The only
* difference is this method adds `UpCast` to input expression to avoid possible runtime
* error caused by type mimatch between serialized column data type and deserializing type.
*/
def deserializerForWithNullSafetyAndUpcast(
expr: Expression,
dataType: DataType,
nullable: Boolean,
walkedTypePath: Seq[String],
funcForCreatingNewExpr: (Expression, Seq[String]) => Expression): Expression = {
funcForCreatingDeserializer: (Expression, Seq[String]) => Expression): Expression = {
val casted = upCastToExpectedType(expr, dataType, walkedTypePath)
deserializerForWithNullSafety(casted, dataType, nullable, walkedTypePath,
funcForCreatingNewExpr)
deserializerForWithNullSafety(casted, nullable, walkedTypePath,
funcForCreatingDeserializer)
}

private def expressionWithNullSafety(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,6 @@ object JavaTypeInference {
s""", name: "$fieldName")""") +: walkedTypePath
val setter = deserializerForWithNullSafety(
path,
dataType,
nullable = nullable,
newTypePath,
(expr, typePath) => deserializerFor(fieldType,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,13 +322,12 @@ object ScalaReflection extends ScalaReflection {
val newTypePath = (s"""- field (class: "$clsName", """ +
s"""name: "$fieldName")""") +: walkedTypePath

// For tuples, we based grab the inner fields by ordinal instead of name.
deserializerForWithNullSafety(
path,
dataType,
nullable = nullable,
newTypePath,
(expr, typePath) => {
// For tuples, we based grab the inner fields by ordinal instead of name.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This location should be more proper for this comment.

if (cls.getName startsWith "scala.Tuple") {
deserializerFor(
fieldType,
Expand Down