refactor #1
refactor #1
Conversation
fqaiser94
left a comment
fqaiser94
left a comment
There was a problem hiding this comment.
Hey @cloud-fan , thanks for doing this.
I'm more than happy to adopt most of these changes.
I see however that this does not pass any of the unit tests. Am I correct in thinking this PR was just your way of sharing your ideas? And that I should fix up the tests/code as necessary after merging it? Please let me know.
| s"Intermediate field ${checkedFields.quoted}" | ||
| } | ||
| TypeCheckResult.TypeCheckFailure( | ||
| s"$fieldName must be struct type, got: ${currentStruct.catalogString}") |
There was a problem hiding this comment.
this code does not check if intermediate fields are struct type. As a result, we get java.lang.ClassCastException thrown by line 593 if any intermediate field is not a struct type. Personally, I would prefer an AnalysisException with a relatively descriptive message like I have in my PR. So I wanted to ask, is this change intentional?
There was a problem hiding this comment.
sorry, made a typo, I've pushed a new commit to fix it.
| case (name, i) => Seq(Literal(name), GetStructField(struct, i, Some(name))) | ||
| }) | ||
| } | ||
| If(IsNull(struct), Literal(null, expr.dataType), expr) |
There was a problem hiding this comment.
I think it would be better if we do something like:
if (struct.nullable) {
If(IsNull(struct), Literal(null, expr.dataType), expr)
} else {
expr
}
Right now, with the code in this PR, WithField will always be nullable even if the original struct was not nullable. This change would fix that.
There was a problem hiding this comment.
It's not a big deal as the optimizer will turn IsNull to false literal if the expression is not nullable. You can see from ScalaReflection that we also add IsNull without checking nullability, to leave optimization work to the optimizer.
| lazy val toCreateNamedStruct: Expression = toCreateNamedStruct(structExpr, attributes) | ||
| case class WithField(struct: Expression, fieldPath: Seq[String], value: Expression) | ||
| extends Unevaluable { | ||
| assert(fieldPath.nonEmpty) |
There was a problem hiding this comment.
Would it not be better to do this in checkInputDataTypes function? So we get an AnalysisException with a descriptive message?
There was a problem hiding this comment.
I do assert here as this should be a bug. Users can't construct an empty fieldPath with public APIs.
|
Yea I didn't fix all the tests. I just tried my idea and ran some local examples. Please help to fix the tests after merging it, thanks! |
2 participants
No description provided.