Numeric range support

[sathishdv]

Description

• Adding Numeric operation suite. Issue 324
• Supports the full suite of arithmetic expressions supported by event-ruler, for example:

{"x": [ { "numeric": [ ">", 0, "<=", 5 ] } ] }

The expression

{"x": [ { "numeric": [ "=", 35 ] } ] }

supports

{"x": 35.0}
{"x": 3.5e1}

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 87.39496% with 30 lines in your changes missing coverage. Please review.

Project coverage is 95.98%. Comparing base (0526acc) to head (7a52da7).

Files with missing lines	Patch %	Lines
range.go	84.61%	10 Missing and 4 partials ⚠️
pattern.go	84.61%	8 Missing and 4 partials ⚠️
numbits.go	88.23%	3 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #403      +/-   ##
==========================================
- Coverage   96.63%   95.98%   -0.65%     
==========================================
  Files          24       25       +1     
  Lines        3179     3414     +235     
==========================================
+ Hits         3072     3277     +205     
- Misses         72       93      +21     
- Partials       35       44       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[timbray]

Wow, this is huge. Thank you! Before I get into a serious review, a few remarks:

1. Could you please post somewhere, here on the PR or in a gist or a blog or anywhere, a small overview of what you did? Were you able to use the aws/event-ruler algorithms or did you have to invent anything new and interesting?
2. I see there are some linting problems which should be easy to fix.
3. The test coverage numbers are down below 90% which is a bit concerning. Among other things, I would expect to pass 100% of the aws/event-ruler unit tests.
4. I took a quick pass through the code and didn't see where you converted ranges to state machines. I remember that in event-ruler, there is a large amount of complex code to do open and closed ranges correctly. It would be wonderful if you figured out a way to do this with much less code. See (1.) in this list, a few words of explanation about how this works would be welcome.
5. @arnehormann is the real expert on numeric representation within Quamina. Arne, could I ask you to have a look at this, perhaps after @sathishdv has provided a bit of introduction to the design?

Once again, thank you @sathishdv for this work!

[sathishdv]

Hi @timbray I will be providing the details soon. Will keep you posted

[timbray]

@sathishdv - OK, I read the code and figured out what you are doing, in valueMatcher and Range.Contains(). I can see that this will produce correct results. Of course correctness is the most important thing, and there is a lot of good work here in parsing the new Pattern syntax and error-checking. Thank you!

However, this is a large departure from the design of Quamina. The basic idea of Quamina is that all the patterns for a field value are compiled together into an NFA and then traversing the NFA for any field tells which patterns matched. This has the result that the behavior is nearly O(1) in the number of rules. This implementation it would be O(N). (Or maybe I'm missing something? Please tell me if so.)

I think I would be reluctant to accept this approach, we had a lot of experience with people who would post hundreds or thousands of rules and they would get a very nasty performance surprise.

However, if you look at the code in aws/event-ruler, you will see how the ranges are compiled into automata. These are quite complex automata. But since you have figured out the fieldMatcher/valueMatcher/smallTable structure, you have done a lot of the work already. I would be very happy if you were to continue and finish the task of building the Range NFAs. My first read through your code made me think I could expect a high-quality implementation.

[timbray]

Because of this loop, this is now O(N) in the number of rules I think?

[sathishdv]

@timbray Thank you so much for reviewing the code and the feedback. Yes you are right this is O(N). Prolly, I've overlooked something from Event-Ruler implementation. I'll work on this and try to improve. Will keep you posted.

[timbray]

Feel free to hit me with questions if things aren't making sense, there's some complicated stuff in there. My email is public. I'd love for Quamina to have those numeric operators.

[timbray]

Feel free to hit me with questions if things aren't making sense, there's some complicated stuff in there. My email is public. I'd love for Quamina to have those numeric operators.

[timbray]

I've added you to the repo as a collaborator so that the CI/CD will automatically run on PR updates.