-
-
Notifications
You must be signed in to change notification settings - Fork 250
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
Implement ObjectStep using a Function1 #2031
Conversation
case class QueryStep[-R](query: ZQuery[R, Throwable, Step[R]]) extends Step[R] | ||
case class StreamStep[-R](inner: ZStream[R, Throwable, Step[R]]) extends Step[R] | ||
|
||
case class ObjectStep[-R](name: String, fields: String => Option[Step[R]]) extends Step[R] |
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 was thinking we could potentially remove Option
and return NullStep
directly to save the allocation. But you won't be able to use orElse
, even though it can be worked around. Any thoughts?
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 was thinking that as well but then hit the .orElse
point. One way would be to check if the step is a NullValue but that could be a legit case of a field returning None.
Another would be using a PartialFunction instead, although I don't think it'd be as performant. I can give it a go
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 PartialFunction seems too messy. Returning NullStep
and then merging fields as below works, but has the caveat I mentioned above
case (ObjectStep(name, fields1), ObjectStep(_, fields2)) =>
ObjectStep(
name,
s =>
fields2(s) match {
case NullStep => fields1(s)
case step => step
}
)
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.
Maybe it's okay since merging arbitrarily keeps one of them anyway (we're not supposed to have the same field twice in theory)?
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 think you're right. The chances that this is a legit usecase are extremely low anyways. Changed fields
to String => Step[R]
instead
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.
Squeezing the performance to the last bit 😄
It seems we've gone a long way around to finally end up here, but I find it very difficult to believe that there is a more optimized version of this 😅
In short, this PR removes all the intermediate creation of
Map
s for us to create an ObjectStep, and instead uses a Function1. This allows us to:ObjectStep
s with aMetadataFunctionStep
Map[String, Step[R]]
In order to avoid breaking source-compatibility when creating ObjectStep, I also added a constructor that takes a
Map[String, Step[R]]
The downside to this change is that in cases where we have aliased fields, the same field will be resolved more than once. However, in practise this is not really a problem because almost always aliased fields are then resolved to a FunctionStep. If we ever see that this might end up being an issue, we can add an intermediate cache, but I highly doubt it will be the case