[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[eliaslevy]

Github user eliaslevy commented on the issue:

https://github.com/apache/flink/pull/3477
  
I created [FLINK-6419](https://issues.apache.org/jira/browse/FLINK-6419).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[kl0u]

Github user kl0u commented on the issue:

https://github.com/apache/flink/pull/3477
  
Hi @eliaslevy , @dawidwys is right, in fact if the state's name is "a" and 
it has 2 matching events, there will be two returned keys "a_0" and "a_1". This 
is definitely counterintuitive and the reason it is still there is for 
backwards compatibility with previous versions. 

I think this should be addressed but I would also suggest to also start a 
discussion in the mailing list to see if people are ok with this. Unfortunately 
we are not aware if anyone is using the `CEP` library at the moment.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[dawidwys]

Github user dawidwys commented on the issue:

https://github.com/apache/flink/pull/3477
  
Right now if the pattern name is "a" the events will be returned with keys: 
"a[0]", "a[1]" and so on, but agree it may be counterintuitive. Please file a 
JIRA for it. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[eliaslevy]

Github user eliaslevy commented on the issue:

https://github.com/apache/flink/pull/3477
  
Am I missing something or is there no way to get access to access to 
multiple events matched by these new quantifiers?  The 
`PatternSelectFunction.select` takes an argument of `Map` and not  
`Map`.  

Which is the one being passed to `select` in the `Map`?  The last matched?  
The first matched?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[dawidwys]

Github user dawidwys commented on the issue:

https://github.com/apache/flink/pull/3477
  
Great! Thanks for your help @kl0u !


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[kl0u]

Github user kl0u commented on the issue:

https://github.com/apache/flink/pull/3477
  
Merged this! Thanks for the work @dawidwys. 
Can you close this PR and the JIRA?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[kl0u]

Github user kl0u commented on the issue:

https://github.com/apache/flink/pull/3477
  
Hi @dawidwys , I have 2 comments for this PR:
1) in the `findFinalStateAfterProceed()` in the `NFA` you should also check 
the filter condition as you traverse the `PROCEED` edges towards the end.
2) In this PR the computation states are consuming, and the `endState` is a 
"dummy" state.
Given this, in the `extractPatternMatches()` in the NFA, you do a 
```
sharedBuffer.extractPatterns(
computationState.getPreviousState().getName(),
computationState.getEvent(),
computationState.getTimestamp(),
computationState.getVersion());
```
to extract the matched patterns.

This will work for patterns that terminate properly, as the `endState` now 
is a "dummy" state and you do not want to present it. But if I understand 
correctly, for timed out patterns, this means that the last state will not be 
presented. Is this correct?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[dawidwys]

Github user dawidwys commented on the issue:

https://github.com/apache/flink/pull/3477
  
I've updated the PR with a solution for the previously failing test. Also 
tried to add some more comments. @kl0u I think it is ready for another round of 
review :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[kl0u]

Github user kl0u commented on the issue:

https://github.com/apache/flink/pull/3477
  
Sounds good! Thanks @dawidwys 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[dawidwys]

Github user dawidwys commented on the issue:

https://github.com/apache/flink/pull/3477
  
Hi, I've addressed the style comments. Still have some doubts about the 
`Preconditions`. I will try to add more comments on the `NFACompiler` with the 
implementation that will handle the failing test case. So I believe it will be 
okay to wait with further reviewing until then.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[kl0u]

Github user kl0u commented on the issue:

https://github.com/apache/flink/pull/3477
  
Hi @dawidwys , the following produces no output, which should not be the 
case. The same holds if you change the pattern to `oneOrMore`.

```
public void testZeroOrMore() {
List inputEvents = new ArrayList<>();

Event startEvent = new Event(40, "c", 1.0);
Event middleEvent1 = new Event(41, "a", 2.0);
Event middleEvent2 = new Event(42, "a", 3.0);
Event middleEvent3 = new Event(43, "a", 4.0);
Event end1 = new Event(44, "b", 5.0);
Event end2 = new Event(45, "d", 6.0);
Event end3 = new Event(46, "d", 7.0);
Event end4 = new Event(47, "e", 8.0);

inputEvents.add(new StreamRecord<>(startEvent, 1));
inputEvents.add(new StreamRecord<>(middleEvent1, 3));
inputEvents.add(new StreamRecord<>(middleEvent2, 4));
inputEvents.add(new StreamRecord<>(middleEvent3, 5));
inputEvents.add(new StreamRecord<>(end1, 6));
inputEvents.add(new StreamRecord<>(end2, 7));
inputEvents.add(new StreamRecord<>(end3, 8));
inputEvents.add(new StreamRecord<>(end4, 9));

Pattern pattern = 
Pattern.begin("start").where(new FilterFunction() {
private static final long serialVersionUID = 
5726188262756267490L;

@Override
public boolean filter(Event value) throws Exception {
return value.getName().equals("a");
}
}).zeroOrMore();

NFA nfa = NFACompiler.compile(pattern, 
Event.createTypeSerializer(), false);

Set resultingPatterns = new HashSet<>();
List allPatterns = new ArrayList<>();

for (StreamRecord inputEvent : inputEvents) {
Collection> patterns = nfa.process(
inputEvent.getValue(),
inputEvent.getTimestamp()).f0;

for (Map foundPattern : patterns) {
System.out.println(foundPattern);
}
}
}
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[kl0u]

Github user kl0u commented on the issue:

https://github.com/apache/flink/pull/3477
  
Thanks @dawidwys I will have a look now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[dawidwys]

Github user dawidwys commented on the issue:

https://github.com/apache/flink/pull/3477
  
Thanks @kl0u for your comments, I've changed all code style specific. 

Some comments were addressing a behaviour that one `Pattern` can be 
translated into multiple "equivalent" `States` with the same name.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #3477: [Flink-3318][cep] Add support for quantifiers to CEP's pa...

[kl0u]

Github user kl0u commented on the issue:

https://github.com/apache/flink/pull/3477
  
Hi @dawidwys , I will start reviewing your PR now. 

I would say that for now it makes sense to have it with exceptions and not 
change the `Pattern` hierarchy. The reason is:
1) not sure what the above split will imply to backwards compatibility
2) there are some additional features that will be nice to have, and they 
all affect also how patterns are defined. A refactoring of the `Pattern` 
hierarchy would make more sense as soon as we have everything in place, and we 
can factor in this knowledge when designing. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---