-
Notifications
You must be signed in to change notification settings - Fork 13.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
Explore to SQL Lab #5101
Explore to SQL Lab #5101
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5101 +/- ##
==========================================
- Coverage 61.31% 61.27% -0.04%
==========================================
Files 369 369
Lines 23497 23534 +37
Branches 2715 2721 +6
==========================================
+ Hits 14407 14421 +14
- Misses 9078 9101 +23
Partials 12 12
Continue to review full report at Codecov.
|
The link should only show for SQLAlchemy datasources. I've been meaning to do something similar from the "View query" modal as well. |
@mistercrunch it only shows for SQLA: https://github.com/apache/incubator-superset/pull/5101/files#diff-39c3f576c74efa400932ceb7eebe8a91R214 I talked with @vylc and she thinks it's more useful to pre-generate the query based on the form data, instead of a plain |
Yup there was some thoughts of showing a "Preview data" tab here as well. |
I changed the "View Query" button to a dropdown label "Query" with 3 options:
For the data preview, we can't reuse the data that is present in |
} | ||
url.searchParams.set('datasourceKey', formData.datasource); | ||
url.searchParams.set('sql', response.query); | ||
window.open(url.href, '_blank'); |
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.
@mistercrunch, is there a better way of doing this? I wasn't sure how to get the base URL from JS.
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.
I think it's a bit the wild west in terms of URL composition in Superset at the moment. It could be good to refactor a src/utils/urls.js
module with constants and utilities for all this.
}; | ||
dispatch(addQueryEditor(queryEditorProps)); | ||
}, | ||
error: () => notify.error(t('The datasource couldn\'t be loaded')), |
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.
nit (optional): the linter allows double quotes when the string contains a single quote, so that you don't have to escape it
import CopyToClipboard from './../../components/CopyToClipboard'; | ||
import { getExploreUrlAndPayload } from '../exploreUtils'; | ||
|
||
import ModalTrigger from './../../components/ModalTrigger'; | ||
import Button from '../../components/Button'; | ||
import { t } from '../../locales'; | ||
|
||
require('react-bootstrap-table/css/react-bootstrap-table.css'); |
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.
nit: import should do the same
@@ -21,6 +25,7 @@ registerLanguage('json', json); | |||
const $ = window.$ = require('jquery'); | |||
|
|||
const propTypes = { | |||
actions: PropTypes.object.isRequired, |
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.
(optional) I think the code has a fair amount of passing the whole actions
object around. I think it's preferable to pass something more generic when possible to make the component more reusable and less bound to the app. Here you could pass the redirectSQLLab
action to an onOpenInEditor
prop.
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.
Good point, will do that.
@@ -211,6 +211,18 @@ export default class DatasourceControl extends React.PureComponent { | |||
/> | |||
</a> | |||
</OverlayTrigger> | |||
{this.props.datasource.type === 'table' && <OverlayTrigger |
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.
nit (optional): personally I like to break before an incomplete tag as in \n<OverlayTrigger
but will defer on the linter here
@@ -152,6 +152,12 @@ def short_data(self): | |||
'creator': str(self.created_by), | |||
} | |||
|
|||
@property |
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 method belongs in connectors.sqla.models.SqlaTable
as Druid datasources don't linke to Database
but to cluster, this base should have a raise NotImplementedError()
or pass
.
Also in that context you'll be able to simply use self.database.select_star(...)
@@ -185,6 +191,8 @@ def data(self): | |||
'metrics': [o.data for o in self.metrics], | |||
'columns': [o.data for o in self.columns], | |||
'verbose_map': verbose_map, | |||
'schema': self.schema, | |||
'select_star': self.select_star, |
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.
Given this here in the base class I'd go for a pass
or return None
in BaseDatasource.select_star
superset/views/core.py
Outdated
@@ -1038,6 +1038,18 @@ def get_viz( | |||
) | |||
return viz_obj | |||
|
|||
@has_access | |||
@expose('/sql') | |||
def sql_from_form_data(self): |
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.
I feel like this endpoint already exists, let me check.
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.
Basically call getExploreUrlAndPayload(formData, endpointType = true)
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.
FYI it gets called when opening the "View Query" while the query is still running or has never run yet.
A few minor comments, otherwise LGTM |
* WIP * Working version * Clean code * Fix lint * Fix unit test; show only for sqla * Working on UX * Dropdown working 66% * Working but needs CSS * Fixed table * Fix lint * Fix unit test * Fix languages path * Fixes * Fix Javascript lint
* WIP * Working version * Clean code * Fix lint * Fix unit test; show only for sqla * Working on UX * Dropdown working 66% * Working but needs CSS * Fixed table * Fix lint * Fix unit test * Fix languages path * Fixes * Fix Javascript lint
This PR adds a button that takes from the explore view to SQLlab, with a pre-populated query on the datasource: