-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Added access check + Druid in endpoint #1224
Conversation
# TODO: move this logic to the model (maybe) | ||
grains_choices = [str(grain.name) for grain in datasource.database.grains()] | ||
# Check if datasource exists | ||
if not datasource: |
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.
we may need to check datasource permissions in other places, can we pull this out into a shareable method?
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 function is defined in BaseView and sharable: https://github.com/airbnb/caravel/pull/1224/files/1cdc6941c6c9639993c3e2c9402ef89748ceae7e#diff-e9b1e371eebef6df66bd8cde507e5931R59
grains = datasource.database.grains() | ||
grains_choices = [grain.name for grain in grains] | ||
elif datasource_class_name == 'DruidDatasource': | ||
time_columns = ['all', '5 seconds', '30 seconds', '1 minute', |
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.
do these time constants get used anywhere else? can we pull them to the top of the file as constants?
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.
Done:)
7ed455f
to
81ce383
Compare
…pache#1205) * create structure for new forked explore view (apache#1099) * create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log * Associate version to entry files (apache#1060) * Associate version to entry files * Modified path joins for configs * Made changes based on comments * Created store and reducers (apache#1108) * Created store and reducers * Added spec * Modifications based on comments * Explore control panel components: Chart control, Time filter, SQL, GroupBy and Filters * Modifications based on comments
905ffd0
to
c7d4385
Compare
LGTM, tests can be added in subsequent PRs. |
c7d4385
to
c7da698
Compare
time_columns = [] | ||
grains_choices = [] | ||
datasource_class_name = datasource_class.__name__ | ||
if datasource_class_name == 'SqlaTable': |
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.
please use datasource_type
time_columns = datasource.dttm_cols | ||
grains = datasource.database.grains() | ||
grains_choices = [grain.name for grain in grains] | ||
elif datasource_class_name == 'DruidDatasource': |
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.
please implement @property
on the datasources in the models.py that will provide you the data you need in the uniform way for both druid and table
@@ -42,6 +42,10 @@ | |||
log_this = models.Log.log_this | |||
can_access = utils.can_access | |||
QueryStatus = models.QueryStatus | |||
DRUID_TIME_GRAINS = [ |
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.
it belongs to models, DruidDatasource class
"metrics": datasource.metrics_combo, | ||
"filter_cols": datasource.filterable_column_names, | ||
} | ||
"datasource_class": datasource_class_name, |
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.
use datasource_type instead of datasource_class here
Done:
Todo:
needs-review @ascott