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

fix Bound filter's empty argument checking for 'lower' and 'upper' #246

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hzhu212
Copy link

@hzhu212 hzhu212 commented Dec 20, 2020

Hi guys, I found a quite obvious bug when I use the Bound filter.

The minimum code to reproduce the bug is as below:

from pydruid.utils.filters import Bound

# I want to select the rows that satisfy "foo >= 0"
Bound(dimension='foo', lower=0).show()

this will cause the following exception:

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-76-afbba87ff9ee> in <module>
      1 from pydruid.utils.filters import Bound
----> 2 Bound(dimension='foo', lower=0).show()

~/Library/Python/3.7/lib/python/site-packages/pydruid/utils/filters.py in __init__(self, dimension, lower, upper, lowerStrict, upperStrict, alphaNumeric, ordering, extraction_function)
    216     ):
    217         if not lower and not upper:
--> 218             raise ValueError("Must include either lower or upper or both")
    219         Filter.__init__(
    220             self,

ValueError: Must include either lower or upper or both

But when I changed lower=0 into lower=1, everything just went well:

Bound(dimension='foo', lower=1).show()

output:

{
    "filter": {
        "type": "bound",
        "dimension": "foo",
        "lower": 1,
        "lowerStrict": false,
        "upper": null,
        "upperStrict": false,
        "alphaNumeric": false,
        "ordering": "lexicographic"
    }
}

Obviously, the root of the problem lies in line 217, which is to check whether argument lower and upper are both omitted:

if not lower and not upper:

However, this checking unexpectedly blocked the number 0, besides None.

Changing this line into the line below should fix the bug:

if lower is None and upper is None:

@pranatishete
Copy link

@hzhu212 This property can be overcome if you pass ordering="numeric" and pass lower and upperbound as 0

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.

None yet

2 participants