Skip to content
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

pat: Fix numeric matching #27

Closed
timbray opened this issue May 27, 2022 · 5 comments
Closed

pat: Fix numeric matching #27

timbray opened this issue May 27, 2022 · 5 comments

Comments

@timbray
Copy link
Owner

timbray commented May 27, 2022

Quamina doesn't know that "30", "3.0e1", and "30.000" are the same number. We already have code to canonicalize numbers with up to 18 digits of precision in the range +/- 10**9 so they can be matched by a DFA, but canonicalizing is runtime-expensive and if you know that all the numbers in your events are integers, purely wasteful.

I think the best way to fix this is to introduce a "numeric" predicate into patterns:

{ "max": [ {"numeric": [ "=", 30] } ] }

But there might be a case for asking at the Match* API level to request canonicalizing numbers in incoming events.

@timbray
Copy link
Owner Author

timbray commented Jun 4, 2022

In fact, the right thing to do here is to add the full suite of numeric pattern features from EventBridge.

@embano1
Copy link
Collaborator

embano1 commented Jun 20, 2022

In fact, the right thing to do here is to add the full suite of numeric pattern features from EventBridge.

Agree. I'd argue any deviation from EB might cause trouble down the road bc I cannot simply reuse EB patterns in Quamina but would have to write custom parsers.

@timbray
Copy link
Owner Author

timbray commented Jun 23, 2024

In fact, the right thing to do here is to add the full suite of numeric pattern features from EventBridge.

Agree. I'd argue any deviation from EB might cause trouble down the road bc I cannot simply reuse EB patterns in Quamina but would have to write custom parsers.

Now that I'm starting to think about this, I note that whenever aws-ruler sees a rule like

{ "x": [ 1, 1.5 ] }

It puts in matchers for both the exact strings 1 and 1.5, but also matchers for the canonicalized forms. So I think that Q has to do the same thing, for compatibility. Then at runtime, if there are any fields in the instance with the canonicalized numbers, whenever it sees a number in the incoming event, it canonicalizes that at match time. Which is quite expensive. And probably purely wasteful for 1 but not 1.5. So I think the right thing to do is do like Ruler but only for non-integer numeric values? Also, should look closely at the canonicalization code and see if it's efficient enough.

@timbray
Copy link
Owner Author

timbray commented Jun 27, 2024

Note that implementing this (semantics are supposed to exactly match Ruler's) led to filing a Ruler issue: aws/event-ruler#163

timbray added a commit that referenced this issue Jul 3, 2024
addresses #27

Signed-off-by: Tim Bray <[email protected]>
timbray added a commit that referenced this issue Jul 9, 2024
* pat: correct numeric matching

addresses #27

Signed-off-by: Tim Bray <[email protected]>

* back off from math/rand/v2 to math/rand for go 1.19

Signed-off-by: Tim Bray <[email protected]>

* patch flaws revealed by concurrency_test.go

Signed-off-by: Tim Bray <[email protected]>

---------

Signed-off-by: Tim Bray <[email protected]>
@timbray
Copy link
Owner Author

timbray commented Jul 10, 2024

Fixed in #330

@timbray timbray closed this as completed Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants