-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
[Bugfix] Druid run_query
dimensions part 3 + Unit tests
#3949
Conversation
superset/connectors/druid/models.py
Outdated
@@ -898,7 +898,7 @@ def run_query( # noqa / druid | |||
row_limit=None, | |||
inner_from_dttm=None, inner_to_dttm=None, | |||
orderby=None, | |||
extras=None, # noqa | |||
extras={}, # noqa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a python gotcha:
http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments,
just set it to None
here and go with extras = extras or {}
in the function body
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
8ba00fc
to
8bed036
Compare
Just seeing if you tested this with a time series chart too? That too was still broken after #3920. |
Yes I checked it with that. The test case is the one where |
I just tested this and it fails for the time series charts. I'm using a column in the group by having the same setup as I reported in #3943. Using a data table view works fine.
|
Looks like it's an issue with |
* Fixed and added tests for druid run query * Fixes style and python3
* Fixed and added tests for druid run query * Fixes style and python3
#3920 (comment)
Hopefully this'll be the last one. Added some unit tests to try to save some face 🥇