Support for anything-but/ignore-case #71

schenksj · 2023-02-04T00:31:24Z

Issue #, if available:

Description of changes:

Add a case-insensitive anything-but construct:

{
  "detail": {
    "command_path": [ { "anything-but": {"equals-ignore-case": [ "c:\windows", "c:\Program Files" ] } } ]
  }
}

Benchmark / Performance (for source code changes):

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running software.amazon.event.ruler.Benchmarks
Reading citylots2
Read 213068 events
EXACT events/sec: 421083.0
WILDCARD events/sec: 298833.1
PREFIX events/sec: 415337.2
SUFFIX events/sec: 418601.2
EQUALS_IGNORE_CASE events/sec: 342553.1
NUMERIC events/sec: 250079.8
ANYTHING-BUT events/sec: 234398.2
ANYTHING-BUT-IGNORE-CASE events/sec: 235955.7
ANYTHING-BUT-PREFIX events/sec: 243506.3
ANYTHING-BUT-SUFFIX events/sec: 237269.5
COMPLEX_ARRAYS events/sec: 6953.7
PARTIAL_COMBO events/sec: 103230.6
COMBO events/sec: 4215.7
Reading citylots2
Read 213068 events
Finding Rules...
Lots: 10000
Lots: 20000
Lots: 30000
Lots: 40000
Lots: 50000
Lots: 60000
Lots: 70000
Lots: 80000
Lots: 90000
Lots: 100000
Lots: 110000
Lots: 120000
Lots: 130000
Lots: 140000
Lots: 150000
Lots: 160000
Lots: 170000
Lots: 180000
Lots: 190000
Lots: 200000
Lots: 210000
Lines: 213068, Msec: 6262
Events/sec: 34025.6
 Rules/sec: 238178.9
Before: 180.3
After: 861.4
Per rule: -1702
Turning JSON into field-lists...
Finding Rules...
Lines: 213068, Msec: 1350
Events/sec: 157828.1
Before: 196.0
After: 633.1
Per rule: -1092
Reading lines...
Finding Rules...
Lots: 10000
Lots: 20000
Lots: 30000
Lots: 40000
Lots: 50000
Lots: 60000
Lots: 70000
Lots: 80000
Lots: 90000
Lots: 100000
Lots: 110000
Lots: 120000
Lots: 130000
Lots: 140000
Lots: 150000
Lots: 160000
Lots: 170000
Lots: 180000
Lots: 190000
Lots: 200000
Lots: 210000
Lines: 213068, Msec: 686
Events/sec: 310594.8
 Rules/sec: 1131186087.5
Reading citylots2
Read 213068 events
Lots: 10000
Lots: 20000
Lots: 30000
Lots: 40000
Lots: 50000
Lots: 60000
Lots: 70000
Lots: 80000
Lots: 90000
Lots: 100000
Lots: 110000
Lots: 120000
Lots: 130000
Lots: 140000
Lots: 150000
Lots: 160000
Lots: 170000
Lots: 180000
Lots: 190000
Lots: 200000
Lots: 210000
Matched: 52527
Lines: 213068, Msec: 9141
Events/sec: 23309.0
Reading lines...
Finding Rules...
Lots: 10000
Lots: 20000
Lots: 30000
Lots: 40000
Lots: 50000
Lots: 60000
Lots: 70000
Lots: 80000
Lots: 90000
Lots: 100000
Lots: 110000
Lots: 120000
Lots: 130000
Lots: 140000
Lots: 150000
Lots: 160000
Lots: 170000
Lots: 180000
Lots: 190000
Lots: 200000
Lots: 210000
Lines: 213068, Msec: 5512
Events/sec: 38655.3
 Rules/sec: 270587.1
DEEP EXACT events/sec: 14285.7
Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 130.768 sec

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

schenksj · 2023-02-04T20:22:35Z

Putting in draft until the issue and proposed syntax is reviewed

baldawar · 2023-02-06T20:48:18Z

Heads down on something this week. I'll be slow in replying. I'm unsure if anything-but-ignore-case is the right solution, but we can wrap up the discussion in #70

schenksj · 2023-02-06T23:24:33Z

@baldawar -- the machine I'm on won't let me update the description, so I'll update the comments when I get on another machine. If you have a sec for feedback, I'd appreciate it!

schenksj · 2023-02-07T00:53:35Z

updated benchmarks and usage comments

baldawar · 2023-02-07T16:20:02Z

Thanks. I'll review this on Thursday (02/09). It wasn't obvious from the tests but if the implementation was a generic ignore-case or specific to anything-but: { "ignore-case" : [...]}. If it's the former, we need more tests for remaining matchers. If its the latter, then we need to document and have open issue on supporting ignore-case for all remaining matchers.

schenksj · 2023-02-07T18:26:53Z

Thanks. I'll review this on Thursday (02/09). It wasn't obvious from the tests but if the implementation was a generic ignore-case or specific to anything-but: { "ignore-case" : [...]}. If it's the former, we need more tests for remaining matchers. If its the latter, then we need to document and have open issue on supporting ignore-case for all remaining matchers.

The PR's implementation is very much specific to anything-but (and not even anything-but/suffix, anything-but/prefix). I'm happy to open an issue unless you'd prefer a core team member to do the analysis?

baldawar · 2023-02-09T17:17:51Z

README.md

+```javascript
+{
+  "detail": {
+    "state": [ { "anything-but": {"ignore-case": [ "Stopped", "OverLoaded" ] } } ]


I didn't spot this earlier but this is different than the existing equals-ignore-case. While I'm on the fence, I think I still prefer using it over creating a new matcher. It's just weird that we'll have this one matcher that works with single value but then with arrays when use in conjunction with anything-but.

@timbray @jonessha any opinions?

agreed, the one-off equals-ignore-case was the original motivation for the creating anything-but-ignore-case... Definitely flexible with this as it has impact for your forward looking vision.

Until Tim & Shawn object by Monday (Shawn is away this week), I'd favor going equals-ignore-case over ignore-case. Definitely will check back by monday EOD.

src/test/software/amazon/event/ruler/ACMachineTest.java

baldawar · 2023-02-09T17:27:07Z

On a high-level, this looks disconnected with rest of ruler's features. I think it'd be great if it creates some similarities with equals-ignore-case and support single value fields (instead of just arrays).

I'm also uncomfortable with the level of complexity AnythingBut is adding within the classes. Its ripe for refactoring, but I'd like that to be initiated only after we're aligned on functionality.

schenksj · 2023-02-09T20:08:25Z

On a high-level, this looks disconnected with rest of ruler's features. I think it'd be great if it creates some similarities with equals-ignore-case and support single value fields (instead of just arrays).

Single-values are allowed (is reuses the parser for anything-but):
https://github.com/schenksj/event-ruler/blob/7ee1c70c9309e0063ddfe9eec962dd29fb80cfb3/src/main/software/amazon/event/ruler/JsonRuleCompiler.java#L445-L458

https://github.com/schenksj/event-ruler/blob/7ee1c70c9309e0063ddfe9eec962dd29fb80cfb3/src/test/software/amazon/event/ruler/JsonRuleCompilerTest.java#L120-L121

schenksj · 2023-02-09T20:09:52Z

I'm also uncomfortable with the level of complexity AnythingBut is adding within the classes. Its ripe for refactoring, but I'd like that to be initiated only after we're aligned on functionality.

Fair! If anything-but itself needs to be re-written, I'll happily close this PR. Just let me know. It is helpful for my product, but not 100% mandatory yet.

baldawar · 2023-02-09T20:39:06Z

I'm also uncomfortable with the level of complexity AnythingBut is adding within the classes. Its ripe for refactoring, but I'd like that to be initiated only after we're aligned on functionality.
Fair! If anything-but itself needs to be re-written, I'll happily close this PR. Just let me know. It is helpful for my product, but not 100% mandatory yet.

If you see it as significant re-write, then definitely don't do it. I'd rather merge the PR as is. Even without considering the unintended time-investment it'll trigger, the coupling of the big rewrites w small feature isn't worth it.

Also, still want to make sure there's no objections on ignore-case vs equals-ignore-case before asking you make any more changes. Looking at the code and test itself, I didn't spot anything that sounded alarming.

Hoping waiting till Monday isn't an issue. Let me know if that's an incorrect assumption.

schenksj · 2023-02-09T21:11:52Z

I'm also uncomfortable with the level of complexity AnythingBut is adding within the classes. Its ripe for refactoring, but I'd like that to be initiated only after we're aligned on functionality.
Fair! If anything-but itself needs to be re-written, I'll happily close this PR. Just let me know. It is helpful for my product, but not 100% mandatory yet.

If you see it as significant re-write, then definitely don't do it. I'd rather merge the PR as is. Even without considering the unintended time-investment it'll trigger, the coupling of the big rewrites w small feature isn't worth it.

Also, still want to make sure there's no objections on ignore-case vs equals-ignore-case before asking you make any more changes. Looking at the code and test itself, I didn't spot anything that sounded alarming.

Hoping waiting till Monday isn't an issue. Let me know if that's an incorrect assumption.

Sounds great! This whole thing has been pretty quick and painless so I'm up for a few more small changes. I can't really commit to help a lot on refactoring the state-machines (which I think would be necessary to really make anything-but less complicated) given all the oddness in existing behavior around applying multiple to a single field. (the ones I mentioned on #69 )

schenksj · 2023-02-11T13:13:31Z

This is a 5 minute search and replace so I'll go ahead and change the syntax to equals-ignore-case and will change it back if you get dissenters.

schenksj · 2023-02-15T17:17:27Z

Hi @baldawar! Anything further on this PR?

baldawar · 2023-02-16T01:33:11Z

Sorry for not getting to this sooner. Just having a busy week. Should be able to merge by tomorrow morning & in maven by afternoon.

baldawar · 2023-02-16T01:33:59Z

In the meantime, as a sanity check, could you update the benchmark results. I'm not expecting anything egregious but just want to make sure it works.

schenksj · 2023-02-16T04:07:56Z

In the meantime, as a sanity check, could you update the benchmark results. I'm not expecting anything egregious but just want to make sure it works.

no problem and done! I thought I had it up to date, but if not it is now :)

baldawar · 2023-02-16T18:46:15Z

https://github.com/aws/event-ruler/releases/tag/v1.2.0

Scott Schenkein added 3 commits February 3, 2023 19:18

added anything-but-ignore-case capability

afceb0c

implemented anything-but-ignore-case

690e716

updated readme and readme tests

f4ceef8

schenksj marked this pull request as draft February 4, 2023 00:32

updated error message

c737d63

Scott Schenkein added 2 commits February 6, 2023 18:15

update to reflect anything-but ignore-case sub-object

749da24

update readme with new anything-but/ignore-case syntax

f0dca1b

schenksj marked this pull request as ready for review February 6, 2023 23:23

schenksj changed the title ~~Support for anything-but-ignore-case~~ Support for anything-but/ignore-case Feb 7, 2023

Scott Schenkein added 3 commits February 6, 2023 19:57

remove extraneous comment

a27b0ae

remove extraneous comment

92a8201

comment cleanup

7ee1c70

baldawar reviewed Feb 9, 2023

View reviewed changes

src/test/software/amazon/event/ruler/ACMachineTest.java Outdated Show resolved Hide resolved

schenksj mentioned this pull request Feb 11, 2023

Support for anything-but/wildcard #72

Closed

Scott Schenkein added 3 commits February 11, 2023 08:14

change anything-but/ignore-case to anything-but/equals-ignore-case

68f7784

updating anything-but/equals-ignore-case syntax

5088a00

updating name of AnythingButEqualsIgnoreCase class

0590ad3

Scott Schenkein added 2 commits February 11, 2023 08:24

fixing typo in anything-but-equals error message

d699e3a

fixing typo in anything-but-equals error message

fe90c3c

baldawar approved these changes Feb 16, 2023

View reviewed changes

baldawar merged commit 3b6202d into aws:main Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for anything-but/ignore-case #71

Support for anything-but/ignore-case #71

schenksj commented Feb 4, 2023 •

edited

Loading

schenksj commented Feb 4, 2023

baldawar commented Feb 6, 2023 •

edited

Loading

schenksj commented Feb 6, 2023

schenksj commented Feb 7, 2023

baldawar commented Feb 7, 2023 •

edited

Loading

schenksj commented Feb 7, 2023

baldawar Feb 9, 2023

schenksj Feb 9, 2023

baldawar Feb 9, 2023

baldawar commented Feb 9, 2023

schenksj commented Feb 9, 2023

schenksj commented Feb 9, 2023

baldawar commented Feb 9, 2023

schenksj commented Feb 9, 2023

schenksj commented Feb 11, 2023

schenksj commented Feb 15, 2023

baldawar commented Feb 16, 2023

baldawar commented Feb 16, 2023

schenksj commented Feb 16, 2023

baldawar commented Feb 16, 2023

Support for anything-but/ignore-case #71

Support for anything-but/ignore-case #71

Conversation

schenksj commented Feb 4, 2023 • edited Loading

Issue #, if available:

Description of changes:

Benchmark / Performance (for source code changes):

schenksj commented Feb 4, 2023

baldawar commented Feb 6, 2023 • edited Loading

schenksj commented Feb 6, 2023

schenksj commented Feb 7, 2023

baldawar commented Feb 7, 2023 • edited Loading

schenksj commented Feb 7, 2023

baldawar Feb 9, 2023

Choose a reason for hiding this comment

schenksj Feb 9, 2023

Choose a reason for hiding this comment

baldawar Feb 9, 2023

Choose a reason for hiding this comment

baldawar commented Feb 9, 2023

schenksj commented Feb 9, 2023

schenksj commented Feb 9, 2023

baldawar commented Feb 9, 2023

schenksj commented Feb 9, 2023

schenksj commented Feb 11, 2023

schenksj commented Feb 15, 2023

baldawar commented Feb 16, 2023

baldawar commented Feb 16, 2023

schenksj commented Feb 16, 2023

baldawar commented Feb 16, 2023

schenksj commented Feb 4, 2023 •

edited

Loading

baldawar commented Feb 6, 2023 •

edited

Loading

baldawar commented Feb 7, 2023 •

edited

Loading