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

feat: Visualize SqlLab.Query model data in Explore 📈 #20281

Merged
merged 102 commits into from
Jul 15, 2022

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Jun 6, 2022

SUMMARY

Explore view now can accept datasource_type = 'query, which represents sql_lab.Query model. Now users will be able to easily chart data from queries being executed in SQL Lab

Screen.Recording.2022-06-10.at.5.39.41.PM.mov

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

  1. Go to Sqllab and run a query
  2. Click Create Chart
  3. Verify that Chart Source is Query with the SQL icon

***OR

  1. Go to Sqllab and run a query
  2. Then run a select id from query
  3. Take one valid id from results returned
  4. Enter the following string into the browser {ephemeral-url}/superset/explore/query/{id}

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@hughhhh hughhhh force-pushed the chart-power-query branch 2 times, most recently from 012e881 to fb58c99 Compare June 6, 2022 20:35
> columns are loading into page
@hughhhh hughhhh force-pushed the chart-power-query branch from fb58c99 to 632ce21 Compare June 7, 2022 17:37
@hughhhh hughhhh force-pushed the chart-power-query branch from 4cb2626 to c8caf51 Compare June 8, 2022 13:29
@@ -29,6 +29,7 @@ export default class DatasourceKey {
this.id = parseInt(idStr, 10);
this.type =
typeStr === 'table' ? DatasourceType.Table : DatasourceType.Druid;
this.type = typeStr === 'query' ? DatasourceType.Query : this.type;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this into a switch statement

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove DatasourceType.Druid

@hughhhh hughhhh force-pushed the chart-power-query branch from c8caf51 to 7d6fd62 Compare June 8, 2022 18:39
lyndsiWilliams and others added 5 commits June 8, 2022 13:53
…ype is Query [41493]

Initial commit to add ability for the a Query Preview Modal to be available when the data source type is Query and not Dataset
Converted ModalTrigger to a functional TypeScript component
> allow for all records to be displayed
> fix select with all columns queries
> filters are now working
@hughhhh hughhhh changed the title Chart-power-query feat: Visualize Query model data in Explore 📈 Jun 10, 2022
@hughhhh hughhhh changed the title feat: Visualize Query model data in Explore 📈 feat: Visualize SqlLab.Query model data in Explore 📈 Jun 10, 2022
@betodealmeida betodealmeida requested review from michael-s-molina and betodealmeida and removed request for michael-s-molina June 10, 2022 17:21
@@ -244,7 +244,13 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
<Icons.DatasetPhysical className="dataset-svg" />
{/* todo(hughhh): move this into a function */}
{datasource.type === DatasourceType.Table && (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be an if and then statement?

@@ -80,6 +80,11 @@ def create(

# pylint: disable=no-self-use
def _convert_to_model(self, datasource: DatasourceDict) -> BaseDatasource:
return ConnectorRegistry.get_datasource(
str(datasource["type"]), int(datasource["id"]), db.session
from superset.dao.datasource.dao import DatasourceDAO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a benefit to importing this in the function versus globally

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are currently getting circuliar dependency issues if we don't wrap the imports in the functions, we have another ticket addressing how to fix this.

return self.extra.get("columns", [])
def columns(self) -> List[ResultSetColumnType]:
# todo(hughhh): move this logic into a base class
from superset.utils.core import GenericDataType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also why import this here?

@hughhhh hughhhh force-pushed the chart-power-query branch from 7d6fd62 to e076f13 Compare June 13, 2022 20:01
@yousoph
Copy link
Member

yousoph commented Jun 13, 2022

/testenv up

@hughhhh hughhhh force-pushed the chart-power-query branch from f58c753 to 15d530c Compare July 14, 2022 22:23
@hughhhh hughhhh requested review from eschutho and AAfghahi July 14, 2022 23:10
@hughhhh hughhhh force-pushed the chart-power-query branch 2 times, most recently from 7a70c8d to 2a57a4f Compare July 15, 2022 00:51
@hughhhh hughhhh force-pushed the chart-power-query branch from 2a57a4f to 555d26b Compare July 15, 2022 00:55
const { useLegacyApi } = metaDataRegistry.get(vizType);
const vizTypeNeedsDataset = useLegacyApi && datasource.type !== 'dataset';

// added boolean column to below show boolean so that the errors aren't overlapping
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

liking all the comments!

@hughhhh hughhhh merged commit e5e8867 into apache:master Jul 15, 2022
@zhaoyongjie
Copy link
Member

zhaoyongjie commented Jul 16, 2022

@hughhhh @yousoph have we really done testing? The explore page of the master branch seems doesn't work at all.

SQLLab -> Explore was broken

explore.is.broken.mov

unable to change datasource

can.not.change.datasource.mov

@hughhhh
Copy link
Member Author

hughhhh commented Jul 18, 2022

@zhaoyongjie I have fixes out for these 2 bugs
#20747
#20751

@@ -152,11 +153,6 @@ def run(self) -> Optional[Dict[str, Any]]:
except (SupersetException, SQLAlchemyError):
dataset_data = dummy_dataset_data

if dataset:
Copy link
Member

@Antonio-RiveroMartnez Antonio-RiveroMartnez Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hughhhh DatasourceEditor uses it, without it you would get the Owners is invalid error every time you try to edit a datasource in explore and you would also see undefined undefined as owner when loading its info, so before putting it back I'd like to ask if there was any particular reason why we removed it?


computed_column["column_name"] = col.get("name")
computed_column["groupby"] = True
columns.append(computed_column)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would it be easier here in the long run to return a column class? columns.append(Column(computed_column)). Then you wouldn't have to worry about type checking every time you reference the property from a dataset vs a query.

@@ -167,8 +173,54 @@ def sql_tables(self) -> List[Table]:
return list(ParsedQuery(self.sql).tables)

@property
def columns(self) -> List[Table]:
return self.extra.get("columns", [])
def columns(self) -> List[ResultSetColumnType]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a situation where you think we may need a setter on this property?


@property
def cache_timeout(self) -> int:
return 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the viz class checks if this is None and if not, it will look at the database. Should we set this to None instead?

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 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 size/XXL 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.