-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add Windowfunction interface to functions api #3324
Conversation
* Process the input. | ||
* @return the output | ||
*/ | ||
O process(Collection<I> input, WindowContext context) throws Exception; |
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 don't know if this is the API we should have for window functions because there is no way to get the topic of individual msg/record in the collection/window. Maybe the collection should be something like Collection<Record> ? so that users can get the topic from which the msg originated from.
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.
In a follow up pr, I was thinking of adding something to WindowContext to get something like getCurrentWindow(or something like that) that returns information in terms of Collection
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.
but the thing is each entry in the collection could be from a different topic. I am not sure how a getCurrentWindow function would work.
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.
If getcurrentWindow returns Collection that should be sufficient. Each Record will have information about the topic of that record.
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.
but collections are not necessarily ordered in Java
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.
that is also not an eloquent API IMO. Users would have to use the index from one list to look up info in another list manually
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.
So the current java.util.function way for handling windowing is not ordered in Java either.
WRT API, if the WindowContext exposes a getCurrentWindow that exposes a Window that should deal with most of the use cases. For the most simple of use cases, just use the collection. For more complicated uses, rely instead on windowContext.getCurrentWindow()
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.
@jerrypeng as discussed, I've updated the interface to reflect our discussions. Please take a look again. Thanks!
rerun integration tests |
@@ -16,7 +16,7 @@ | |||
* specific language governing permissions and limitations | |||
* under the License. | |||
*/ | |||
package org.apache.pulsar.functions.windowing; | |||
package org.apache.pulsar.functions.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.
this is an API breaking change. can we document that?
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.
@sijie This is not api breaking change. Previously WindowContext was not exposed to the user at all. It was just implementation detail. Now we are exposing this.
I will add up documentation as part of the another pr because there are other changes on top of this.
Motivation
Currently we support windowed functions but these functions have to be java.util.Function. However simple java.util.Function cannot fetch any kind of meta data about the function since it lacks any kind of context handle. This pr introduces the WindowFunction as anologous to the Function interface which exposes the Context interface.
Modifications
Describe the modifications you've done.
Result
After your change, what will change.