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

The API of inclusive/exclusive ranges in select/3 is a bit problematic #3

Closed
Qqwy opened this issue Jul 27, 2019 · 7 comments
Closed

Comments

@Qqwy
Copy link

Qqwy commented Jul 27, 2019

Currently, changing whether min_key: and max_key: are inclusive or exclusive is changed by wrapping the key you want to match on by {your_key, :excluded}.

I think this interface could be improved, since it conflicts when someone tries to e.g. use a {:key, :excluded} as key. Also, it will silently fail (and not match any keys it was expected to match) if someone mistypes :excluded.

I think a better approach would be to have extra options to toggle between inclusive and exclusive, for instance named min_key_inclusive: true, or min_key_settings: [exclusive: true].

@lucaong
Copy link
Owner

lucaong commented Jul 30, 2019

Hi, thanks a lot for opening a discussion on this.

I thought about it too. Using the long form should be always safe (in case the key is literally {:xyz, :excluded} the long form would be {{:xyz, :excluded}, :included}), but I agree it’s not ideal. Also, yes, misspellings should cause explicit errors.

Your idea to use min_key_exclusive: sounds good. I just wonder if min_key: should be inclusive by default, or if it’s better to be explicit and only accept either min_key_inclusive or min_key_exclusive. I slightly lean for the first option (min_key or min_key_exclusive), because I suspect that the inclusive case is the most common and a reasonable default.

What’s your opinion?

@Qqwy
Copy link
Author

Qqwy commented Jul 30, 2019

I agree. Half-open ranges (so inclusive min-key and exclusive max-key) are, as far as I am aware, the most wide-spread and commonly used ranges in computing science and software engineering.

@lucaong
Copy link
Owner

lucaong commented Jul 30, 2019

What I mean, is something like this:

CubDB.select(db, [
  min_key: :foo,
  max_key_exclusive: :bar
])

@Qqwy
Copy link
Author

Qqwy commented Jul 30, 2019

Ah!

I had a slightly different idea in mind, namely:

CubDB.select(db, [
  min_key: {:foo, 42},
  min_key_exclusive: true,
  max_key: {:foo, 1234},
  max_key_exclusive: false
])

or possibly:

CubDB.select(db, [
  min_key: {:foo, 42},
  min_key_settings: [exclusive: true],
  max_key: {:foo, 1234},
  max_key_settings: [exclusive: false]
])

What do you think?

@lucaong
Copy link
Owner

lucaong commented Jul 31, 2019

Adding to this discussion, right now there is another issue related with the min_key and max_key options. The problem is that min_key: nil currently means no minimum key, while if one wants a minimum key which is literally nil, the long form must be used: min_key: {nil, :included}. This is a bit confusing, and one more reason for a refactoring of the select/3 options before v1.0.

The ideal solution would be both intuitive and free of corner cases.

The best possibilities, in my view, are the following:

  1. Forcing always the long form, min_key: {key, :included}. The advantages would be no corner cases (bot nil and {:abc, :excluded} would be valid values for key), and errors could be thrown if the option format is invalid (e.g. if misspelling :included or :excluded, or if using anything else than a 2-elements tuple). The disadvantage is a slightly more cumbersome (but explicit) syntax.
# Option 1:
{:ok, result} = CubDB.select(db, [
  min_key: {:foo, :included},
  max_key: {:bar, :excluded}
]))
  1. Only allowing the short form, but making it possible to use min_key: key/max_key: key for the inclusive case, and min_key_exclusive: key/max_key_exclusive: key for the exclusive case. Setting min_key: nil would mean a literal nil value for the minimum key, and for open ranges one would have to omit min_key/max_key. The advantage would be no corner cases (again, key could be any term, including nil and {:something, :included}), and a somehow simple syntax. The disadvantage would be the need of two different option names for inclusive or exclusive cases, plus the need to handle the error case when both min_key and min_key_exclusive are passed.
# Option 2:
{:ok, result} = CubDB.select(db, [
  min_key: :foo,
  max_key_exclusive: :bar
]))

Adding more options is also a possibility, but I would prefer to keep it simpler.

I am undecided on the options above, happy to take feedback, and I will need to sleep on it in order to come to a decision 🙂

@lucaong lucaong changed the title The API of inclusive/exclusive ranges in selects is a bit problematic The API of inclusive/exclusive ranges in select/3 is a bit problematic Jul 31, 2019
@lucaong lucaong changed the title The API of inclusive/exclusive ranges in select/3 is a bit problematic The API of inclusive/exclusive ranges in select/3 is a bit problematic Jul 31, 2019
@lucaong
Copy link
Owner

lucaong commented Sep 11, 2019

After (a lot of) thinking, I feel that your proposal is the clearest and simplest:

 # Boundaries are inclusive by default
{:ok, result} = CubDB.select(db, [
  min_key: {:foo, 123},
  max_key: {:bar, 321}
]))

# Setting min/max key to nil means literal nil
{:ok, result} = CubDB.select(db, [
  min_key: nil,
  max_key: {:bar, 321}
]))

# To leave the range open, omit min/max key
{:ok, result} = CubDB.select(db, [
  max_key: {:bar, 321}
]))

# Make boundaries exclusive by setting min/max_key_inclusive to false
{:ok, result} = CubDB.select(db, [
  min_key: {:foo, 123},
  max_key: {:bar, 321},
  max_key_inclusive: false
]))

I will soon implement this. It will be a breaking change, but it's good to improve this part of the API before version 1.

Thanks a lot for the feedback!

@lucaong
Copy link
Owner

lucaong commented Sep 11, 2019

The new version v0.13.0 is published on Hex, introducing the improved API.

@lucaong lucaong closed this as completed Sep 11, 2019
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