Skip to content
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

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Struct scala #262

merged 1 commit into from
Oct 17, 2023

Conversation

honnix
Copy link
Member

@honnix honnix commented Oct 11, 2023

TL;DR

On top of #259 to support Scala case class as struct.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Basically to support this type of IO:

case class NestedNestedNested(string: String)
case class NestedNested(double: Double, nested: NestedNestedNested)
case class Nested(
    boolean: Boolean,
    byte: Byte,
    short: Short,
    int: Int,
    long: Long,
    float: Float,
    double: Double,
    string: String,
    list: List[String],
    map: Map[String, String],
    optBoolean: Option[Boolean],
    optByte: Option[Byte],
    optList: Option[List[String]],
    optMap: Option[Map[String, String]],
    nested: NestedNested
)

case class NestedIOTaskInput(
    @Description("the name of the person to be greeted")
    name: SdkBindingData[String],

    @Description("a nested input")
    generic: SdkBindingData[Nested]
)

Tracking Issue

NA

Follow-up issue

NA

@honnix honnix changed the base branch from master to struct October 11, 2023 14:43
@honnix honnix force-pushed the struct-scala branch 4 times, most recently from ecd3aaf to c90241c Compare October 11, 2023 15:56
@honnix honnix marked this pull request as ready for review October 12, 2023 09:41
@honnix honnix force-pushed the struct-scala branch 3 times, most recently from 70fe1c4 to 436e893 Compare October 14, 2023 19:36
Base automatically changed from struct to blob October 17, 2023 08:09
@honnix honnix mentioned this pull request Oct 17, 2023
8 tasks
Signed-off-by: Hongxin Liang <[email protected]>
Comment on lines +28 to +46
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
)
Copy link
Collaborator

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?

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 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.

Copy link
Member Author

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.

Copy link
Collaborator

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

Comment on lines +236 to +237
// more specific matching to fail the usage of SdkBindingData[Option[_]]
implicit def optionLiteralType: SdkScalaLiteralType[Option[_]] = ???
Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Collaborator

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 _ => ???
}

Copy link
Member Author

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.

@andresgomezfrr andresgomezfrr merged commit 76fb3be into blob Oct 17, 2023
4 checks passed
@andresgomezfrr andresgomezfrr deleted the struct-scala branch October 17, 2023 12:27
honnix added a commit that referenced this pull request Oct 17, 2023
andresgomezfrr pushed a commit that referenced this pull request Oct 17, 2023
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants