You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/09/23 18:46:03 UTC
[GitHub] [flink] zentol opened a new pull request, #20893: [FLINK-29403][cep] Streamline SimpleCondition usage
zentol opened a new pull request, #20893:
URL: https://github.com/apache/flink/pull/20893
Add `SimpleCondition.of(FilterFunction<T>)` and adjust all tests and docs accordingly.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] SteNicholas commented on a diff in pull request #20893: [FLINK-29403][cep] Streamline SimpleCondition usage
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #20893:
URL: https://github.com/apache/flink/pull/20893#discussion_r979133150
##########
flink-libraries/flink-cep/src/main/java/org/apache/flink/cep/pattern/conditions/SimpleCondition.java:
##########
@@ -40,4 +40,13 @@
public boolean filter(T value, Context<T> ctx) throws Exception {
return filter(value);
}
+
+ public static <T> SimpleCondition<T> of(FilterFunction<T> filters) {
Review Comment:
@zentol, IMO, it's better to deprecate the `SimpleCondition` class and replace the `Condition` with the `FilterFunction` because other condition implementation should follow this usage to add `of(filterFunction)`. The streamline SimpleCondition usage is a temporary improvement of `Condtion` usage.
cc @dianfu
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] alpinegizmo commented on a diff in pull request #20893: [FLINK-29403][cep] Streamline SimpleCondition usage
Posted by GitBox <gi...@apache.org>.
alpinegizmo commented on code in PR #20893:
URL: https://github.com/apache/flink/pull/20893#discussion_r984666197
##########
flink-libraries/flink-cep/src/main/java/org/apache/flink/cep/pattern/conditions/SimpleCondition.java:
##########
@@ -40,4 +40,13 @@
public boolean filter(T value, Context<T> ctx) throws Exception {
return filter(value);
}
+
+ public static <T> SimpleCondition<T> of(FilterFunction<T> filters) {
Review Comment:
This is already a big improvement in usability, and I don't see how to do better without introducing a breaking (or confusing) change.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] SteNicholas commented on a diff in pull request #20893: [FLINK-29403][cep] Streamline SimpleCondition usage
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #20893:
URL: https://github.com/apache/flink/pull/20893#discussion_r979133150
##########
flink-libraries/flink-cep/src/main/java/org/apache/flink/cep/pattern/conditions/SimpleCondition.java:
##########
@@ -40,4 +40,13 @@
public boolean filter(T value, Context<T> ctx) throws Exception {
return filter(value);
}
+
+ public static <T> SimpleCondition<T> of(FilterFunction<T> filters) {
Review Comment:
@zentol, IMO, it's better to deprecate the `SimpleCondition` class and replace the `Condition` with the `FilterFunction`. The streamline SimpleCondition usage is temporiary improvement of `Condtion` usage.
cc @dianfu
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] SteNicholas commented on a diff in pull request #20893: [FLINK-29403][cep] Streamline SimpleCondition usage
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #20893:
URL: https://github.com/apache/flink/pull/20893#discussion_r979133150
##########
flink-libraries/flink-cep/src/main/java/org/apache/flink/cep/pattern/conditions/SimpleCondition.java:
##########
@@ -40,4 +40,13 @@
public boolean filter(T value, Context<T> ctx) throws Exception {
return filter(value);
}
+
+ public static <T> SimpleCondition<T> of(FilterFunction<T> filters) {
Review Comment:
@zentol, IMO, it's better to deprecate the `SimpleCondition` class and replace the `Condition` with the `FilterFunction`.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #20893: [FLINK-29403][cep] Streamline SimpleCondition usage
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #20893:
URL: https://github.com/apache/flink/pull/20893#issuecomment-1256555052
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "2c7d41b3202605d407993c12fec8aa85e8aa22ac",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "2c7d41b3202605d407993c12fec8aa85e8aa22ac",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 2c7d41b3202605d407993c12fec8aa85e8aa22ac UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20893: [FLINK-29403][cep] Streamline SimpleCondition usage
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20893:
URL: https://github.com/apache/flink/pull/20893#issuecomment-1259285829
CI failure caused by a bug in the production tests; filed https://issues.apache.org/jira/browse/FLINK-29422.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #20893: [FLINK-29403][cep] Streamline SimpleCondition usage
Posted by GitBox <gi...@apache.org>.
zentol commented on code in PR #20893:
URL: https://github.com/apache/flink/pull/20893#discussion_r984453514
##########
flink-libraries/flink-cep/src/main/java/org/apache/flink/cep/pattern/conditions/SimpleCondition.java:
##########
@@ -40,4 +40,13 @@
public boolean filter(T value, Context<T> ctx) throws Exception {
return filter(value);
}
+
+ public static <T> SimpleCondition<T> of(FilterFunction<T> filters) {
Review Comment:
At the end of the day this doesn't need to be the final solution.
While `Pattern#where(FilterFunction)` is a lot more user-friendly this would be a breaking change, because an existing `SimpleCondition` would satisfy both signatures which the compiler rejects.
I'd like to avoid using different method names for the `FilterFunction` variants.
We could also modify `SimpleCondition` to not implement the `FilterFunction` interface, but this would again be a potentially breaking change.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #20893: [FLINK-29403][cep] Streamline SimpleCondition usage
Posted by GitBox <gi...@apache.org>.
zentol commented on code in PR #20893:
URL: https://github.com/apache/flink/pull/20893#discussion_r978975202
##########
flink-libraries/flink-cep/src/main/java/org/apache/flink/cep/pattern/conditions/SimpleCondition.java:
##########
@@ -40,4 +40,13 @@
public boolean filter(T value, Context<T> ctx) throws Exception {
return filter(value);
}
+
+ public static <T> SimpleCondition<T> of(FilterFunction<T> filters) {
Review Comment:
Alternatively we could deprecate this entire class and add `Pattern#where`/`or` variations that accept a `FilterFunction`.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #20893: [FLINK-29403][cep] Streamline SimpleCondition usage
Posted by GitBox <gi...@apache.org>.
zentol commented on code in PR #20893:
URL: https://github.com/apache/flink/pull/20893#discussion_r980213947
##########
flink-libraries/flink-cep/src/main/java/org/apache/flink/cep/pattern/conditions/SimpleCondition.java:
##########
@@ -40,4 +40,13 @@
public boolean filter(T value, Context<T> ctx) throws Exception {
return filter(value);
}
+
+ public static <T> SimpleCondition<T> of(FilterFunction<T> filters) {
Review Comment:
> replace the `Condition` with the `FilterFunction`
I'm not sure I understand. Which `Condition` to do you want to _replace_?
> other condition implementation should follow this usage
I'm not sure this is needed; AFAICT all other IterativeCondition sub-classes are truly internal.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] SteNicholas commented on a diff in pull request #20893: [FLINK-29403][cep] Streamline SimpleCondition usage
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #20893:
URL: https://github.com/apache/flink/pull/20893#discussion_r979133150
##########
flink-libraries/flink-cep/src/main/java/org/apache/flink/cep/pattern/conditions/SimpleCondition.java:
##########
@@ -40,4 +40,13 @@
public boolean filter(T value, Context<T> ctx) throws Exception {
return filter(value);
}
+
+ public static <T> SimpleCondition<T> of(FilterFunction<T> filters) {
Review Comment:
@zentol, IMO, it's better to deprecate the `SimpleCondition` class and replace the `Condition` with the `FilterFunction`. The streamline SimpleCondition usage is a temporary improvement of `Condtion` usage.
cc @dianfu
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org