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 the mapbox for Druid #725

Merged
merged 1 commit into from
Jul 13, 2016
Merged

Fix the mapbox for Druid #725

merged 1 commit into from
Jul 13, 2016

Conversation

x4base
Copy link
Contributor

@x4base x4base commented Jul 7, 2016

Currently when we try to use mapbox on a Druid datasource, it will show 'Invalid Choice' error of SelectField and "query() got an unexpected keyword argument 'columns'" due to the inconsistent interface between SqlTable and DruidDatasource. With this patch, mapbox still won't work on Druid, because you can not select columns in Druid queries. But at least it helps reducing some errors.

If I want to move one step forward, what can I do to make it work on Druid? Should I use metrics_combo as the choices of all_columns_x and all_columns_y instead of column_names?

@coveralls
Copy link

coveralls commented Jul 7, 2016

Coverage Status

Coverage remained the same at 81.036% when pulling 0e5b5cd on x4base:fix_mapbox into 8135c24 on airbnb:master.

@x4base
Copy link
Contributor Author

x4base commented Jul 9, 2016

OK, I've just noticed the dicussion in #707. Does it mean this is not needed because mapbox is going to be disabled?

@mistercrunch
Copy link
Member

mistercrunch commented Jul 11, 2016

For the record, @LAlbertalli isn't a core committer, he's turning the feature off in his own production deployment. We're committed to the feature but it may need some tweaks.

To make the feature work for Druid, we need to implement the columns feature of the query interface to perform a Select type query (unaggregated) against druid. I started it in a branch and got stuck along the way, it seemed like PyDruid had issues with Select queries and didn't get to the bottom of it.

@LAlbertalli
Copy link
Contributor

@mistercrunch , yes, sorry if my statement was misleading. I was speaking of our internal branch.

@mistercrunch mistercrunch merged commit 09c95fb into apache:master Jul 13, 2016
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0 labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants