-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Struct scala #262
Struct scala #262
Conversation
ecd3aaf
to
c90241c
Compare
70fe1c4
to
436e893
Compare
Signed-off-by: Hongxin Liang <[email protected]>
case class Nested( | ||
boolean: Boolean, | ||
byte: Byte, | ||
short: Short, | ||
int: Int, | ||
long: Long, | ||
float: Float, | ||
double: Double, | ||
string: String, | ||
list: List[String], | ||
listOfNested: List[NestedNested], | ||
map: Map[String, String], | ||
mapOfNested: Map[String, NestedNested], | ||
optBoolean: Option[Boolean], | ||
optByte: Option[Byte], | ||
optList: Option[List[String]], | ||
optMap: Option[Map[String, String]], | ||
nested: NestedNested | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering now... what happens if you try to build a nested input using outputs from another task? I guess this is not possible because the outputs will be SdkBindingData[_]
and the nested input uses plain types, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was discussed briefly internally before, and I don't think there is any valid use case could make it troublesome.
Assuming output is foo = SdkBindingData[Foo]
, to use the value in a nested structure, it would be just Bar(foo=foo)
, where Bar
is class Bar(foo: SdkBindingData[Foo])
. They key point is: nested structure is always plain type, no matter input or output, so they should always match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is not something special to Scala and it is the same for Java API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is not something special to Scala and it is the same for Java API.
I know, I know, but I'm wondering it while I was seeing the scala case class hahaha
// more specific matching to fail the usage of SdkBindingData[Option[_]] | ||
implicit def optionLiteralType: SdkScalaLiteralType[Option[_]] = ??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we support this in the future, it will be well received by scala users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether it makes sense to support null
value for protobuf message. There is no real null
in protobuf, and protobuf struct supports null value in a special way. If user passes in None
, we will use default value for serialization, and when the task receives that, it needs to understand whether the input was actually None
, or was it really an empty string for example.
I have not thought about this in depth, but right now I don't see much value of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it doesn't offer so much value, only better scala experience for scala developers who normally use Option with pattern matching. But I think they could do something like:
Option(input.myField.get) match {
case Some(_) => ???
case _ => ???
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I hope they normally do not need to use "null value" for signalling because it is not a good practice using protobuf.
Signed-off-by: Hongxin Liang <[email protected]>
* Bring back Blob support Signed-off-by: Hongxin Liang <[email protected]> * Remove BlobTypeDescription Signed-off-by: Hongxin Liang <[email protected]> * Add blob back Signed-off-by: Hongxin Liang <[email protected]> * Blob in list and map Signed-off-by: Hongxin Liang <[email protected]> * Clean up Signed-off-by: Hongxin Liang <[email protected]> * Support struct (#259) Signed-off-by: Hongxin Liang <[email protected]> * Support struct in Scala layer (#262) Signed-off-by: Hongxin Liang <[email protected]> --------- Signed-off-by: Hongxin Liang <[email protected]>
TL;DR
On top of #259 to support Scala case class as struct.
Type
Are all requirements met?
Complete description
Basically to support this type of IO:
Tracking Issue
NA
Follow-up issue
NA