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

Allow excluding min/max in DoubleRangeFilter #1260

Closed
adumovic opened this issue Sep 11, 2020 · 4 comments · Fixed by #1268
Closed

Allow excluding min/max in DoubleRangeFilter #1260

adumovic opened this issue Sep 11, 2020 · 4 comments · Fixed by #1268
Milestone

Comments

@adumovic
Copy link

adumovic commented Sep 11, 2020

Is your feature request related to a problem? Please describe.
We have a need to subdivide a continuous variable into ranges that are even and predicable, say, 0.0 - 4.0 into 4 buckets:
[0.0, 1.0) [1.0, 2.0), [2.0, 3.0), [3.0, 4.0)
Each bucket with these boundaries would have no overlap. The current inclusive/inclusive boundaries make this very difficult to do without trying to hand make the boundaries to be hopefully close enough but not equal to the next value that there will not be another value in between the ending of one bucket and the start of the next one ie:
[0.0, 0.99999999...], [1, 1.99999999...]

We also have used this type of representation for discrete values as well (like enumerations) where it is even more important that the buckets do not overlap.

Describe the solution you'd like
We propose no default changes to how things work, by default a query would still run with min/max as inclusive boundaries.

However, we propose adding a new field to the DoubleRangeFilter contract that introduces the idea that either or both bounds could be exclusive:

message DoubleRangeFilter {
  // Name of the ticket's search_fields.double_args this Filter operates on.
  string double_arg = 1;

  // Maximum value.
  double max = 2;

  // Minimum value.
  double min = 3;

  enum Exclude {
    NONE = 0;
    MIN = 1;
    MAX = 2;
    BOTH = 3;
  }
  Exclude exclude = 4;
}

This will enable all of the scenarios we currently use and more

Describe alternatives you've considered
An alternative to achieve this is attempt to understand the minimum value between the discrete bucket ranges and do some math that will hopefully be close enough to the next buckets minimum range that there is no possibility of a value existing between it. The main concern here is that the computation of these ranges may be done on different platform architectures and may result in slightly varying results at high precision:
So instead of the desired list of filters for a few non overlapping queries:
[0.0, 1.0) [1.0, 2.0), [2.0, 3.0), [3.0, 4.0)
We could try:
[0.0, 0.99999999...) [1.0, 1.9999999...), [2.0, 2.9999999...), [3.0, 3.99999999...)

This has the potential to miss some values between the max of one range and the next range, in addition to being cumbersome to compute and look at compared to the mathematically concise way of defining these.

Additional context
We currently run a matchmaker that we would like to port over to using open match as the ticket backend; our existing logic for generating queries that is exposed to customers has the behavior we describe, and it would be a breaking change we would like to avoid to have to convey to customers that they will now need to simulate ranges like this.

@Laremere
Copy link

The problem statement and general approach are good here.

Instead of an enum, just using two bools seems simpler to me:

message DoubleRangeFilter {
  // Name of the ticket's search_fields.double_args this Filter operates on.
  string double_arg = 1;

  // Maximum value.
  double max = 2;

  // Minimum value.
  double min = 3;

  bool exclusive_max = 4;

  bool exclusive_min = 5;

Thoughts?

@adumovic
Copy link
Author

I am not very familiar with the conventions of protobuf. I am fine with whatever you recommend as most consistent with conventions/existing code.

@aLekSer
Copy link
Collaborator

aLekSer commented Sep 12, 2020

I can implement this small enhancement. I will make a draft, not sure if this change to protobuf would be a breaking change.
Found a useful tool which provides compatibility checking https://buf.build/docs/tour-5. This tool also provide protobuf linter support.

@Laremere
Copy link

Hey @aLekSer
I believe Unity mentioned elsewhere that they were already working on this change - just want to make sure people aren't doing double work here.

As for the backwards compatibility, adding these fields is good, though we will want to mark them as a beta feature for a release or two.

Both enums and bools are reasonable to use in Protos - my suggestion is to merely decouple the states. Seems easier to reason about to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants