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

fixed #1879: Add support for range filter #1972

Closed
wants to merge 1 commit into from
Closed

fixed #1879: Add support for range filter #1972

wants to merge 1 commit into from

Conversation

zhxiaogg
Copy link
Contributor

Currently it supports the following schema:

{
  "type": "between",
  "numerically": false,
  "upper": "abd",
  "lower": "abc"
}

The numerically field can be omitted and defaults to be false if both upper and lower are not numbers. It means the omitted field will be true if given:

{
  "type": "between",
  "upper": 1,
  "lower": 0.1
}

and will be false if given:

{
  "type": "between",
  "upper": "1",
  "lower": "0.1"
}

Also worth note:

  1. the bounds to defaults to value <= upper && value >= lower and does NOT support configuration
  2. string comparison are implemented use String#compareTo which is case sensitive.

Finally, I just finished coding and have NOT tested the code. Hopefully somebody could help to review this code and feed me back some suggestions.

@drcrallen
Copy link
Contributor

Most boundaries in druid are "greater than or equal" on the lower and "less than" on the upper. Can you give some examples of if that would or would not work for this?

@JsonProperty("dimension") @Nonnull String dimension,
@JsonProperty("lower") @Nonnull Object lower,
@JsonProperty("upper") @Nonnull Object upper,
@JsonProperty("numerically") Boolean numerically
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be boolean in which case it will default to false

@drcrallen
Copy link
Contributor

@zhxiaogg Have you seen #1936 ?

@zhxiaogg
Copy link
Contributor Author

@drcrallen yes, i just noticed that... oops!

@zhxiaogg zhxiaogg closed this Nov 16, 2015
@zhxiaogg
Copy link
Contributor Author

@drcrallen can you still review this code? I'll pick a new issue later.

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

Successfully merging this pull request may close these issues.

2 participants