grundprinzip commented on code in PR #38970:
URL: https://github.com/apache/spark/pull/38970#discussion_r1042789887
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -518,9 +518,16 @@ class SparkConnectPlanner(session: SparkSession) {
}
private def transformCast(cast: proto.Expression.Cast): Expression = {
- Cast(
- transformExpression(cast.getExpr),
- DataTypeProtoConverter.toCatalystType(cast.getCastToType))
+ cast.getCastToTypeCase match {
+ case proto.Expression.Cast.CastToTypeCase.TYPE =>
+ Cast(
+ transformExpression(cast.getExpr),
+ DataTypeProtoConverter.toCatalystType(cast.getType))
+ case _ =>
+ Cast(
+ transformExpression(cast.getExpr),
+ session.sessionState.sqlParser.parseDataType(cast.getTypeStr))
Review Comment:
Wouldn't it be nice if we could just add a second string argument to the
`Cast` expression so it could resolved the expression automatically? Would this
make the design easier? Because then you wouldn't even need a custom expression
for cast.
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -43,7 +43,11 @@ message Expression {
Expression expr = 1;
// (Required) the data type that the expr to be casted to.
- DataType cast_to_type = 2;
+ oneof cast_to_type {
Review Comment:
FYI: this is a breaking change in the proto message. Ideally, we would use
```
reserved 2;
oneof cast_to_type {
DataType type = 3;
string type_str = 4;
}
```
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -43,7 +43,11 @@ message Expression {
Expression expr = 1;
// (Required) the data type that the expr to be casted to.
- DataType cast_to_type = 2;
+ oneof cast_to_type {
+ DataType type = 2;
+ // If this is set, Server will use Catalyst parser to parse this string
to DataType.
+ string type_str = 3;
Review Comment:
This comes back to the discussion that I had in my draft PR if the second
argument to the `cast()` function should be modeled as an expression or not.
Right now it takes a value but could be modeled as a string as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
