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

WIP: Add Explore Database to CTAS/CVAS visualization flow #19

Closed
wants to merge 1 commit into from

Conversation

JasonD28
Copy link
Contributor

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@JasonD28 JasonD28 marked this pull request as draft June 26, 2020 02:01
@github-actions
Copy link

WARNING: Prefer TypeScript

It looks like your PR contains new .js or .jsx files. As decided in SIP-36, all new files should be written in TypeScript. Please convert new JavaScript files to TypeScript and then re-request review.

@codecov
Copy link

codecov bot commented Jun 26, 2020

Codecov Report

Merging #19 into master will increase coverage by 5.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
+ Coverage   65.81%   70.84%   +5.03%     
==========================================
  Files         594      404     -190     
  Lines       31319    13056   -18263     
  Branches     3206     3206              
==========================================
- Hits        20613     9250   -11363     
+ Misses      10525     3690    -6835     
+ Partials      181      116      -65     
Flag Coverage Δ
#cypress 53.55% <ø> (?)
#javascript 59.69% <ø> (+<0.01%) ⬆️
#python ?
Impacted Files Coverage Δ
...erset-frontend/src/SqlLab/components/ResultSet.jsx 71.68% <ø> (ø)
...src/components/FilterableTable/FilterableTable.tsx 81.77% <0.00%> (-0.41%) ⬇️
superset/db_engines/hive.py
superset/translations/utils.py
superset/utils/logging_configurator.py
superset/dashboards/commands/delete.py
superset/utils/core.py
superset/dashboards/api.py
superset/views/key_value.py
superset/views/chart/views.py
... and 326 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be936c2...a9fec93. Read the comment docs.

Copy link
Contributor

@bkyryliuk bkyryliuk left a comment

Choose a reason for hiding this comment

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

couple small nits

@@ -187,6 +187,10 @@ def allows_cost_estimate(self) -> bool:
and cost_estimate_enabled
)

@property
def explore_database_visualization_id(self) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

lets keep it simple, just do explore_database_id

also change code https://github.com/apache/incubator-superset/blob/c7618ee54b62760ac5f738996379de0dcd5e326e/superset/views/database/views.py#L190 here to use this property

@@ -135,6 +135,7 @@ class DatabaseRestApi(DatabaseMixin, BaseSupersetModelRestApi):
"allow_csv_upload",
"allows_subquery",
"allows_cost_estimate",
"explore_database_visualization_id",
Copy link
Contributor

Choose a reason for hiding this comment

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

let's keep it simple, just use explore_database_id

@@ -238,7 +238,7 @@ export default class ResultSet extends React.PureComponent {
<ExploreCtasResultsButton
table={tmpTable}
schema={tmpSchema}
dbId={query.dbId}
dbId={this.props.database.explore_database_visualization_id}
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about doing that in the api endpoint, however this is even cleaner approach

@JasonD28 JasonD28 closed this Jul 10, 2020
@JasonD28 JasonD28 deleted the explore_db_visual branch July 10, 2020 17:46
bkyryliuk pushed a commit that referenced this pull request Dec 9, 2021
Add new core modules from incubator-superset
bkyryliuk pushed a commit that referenced this pull request Dec 9, 2021
* fix: show only necessary ticks on log scale and add storybook

* fix: storybook path
bkyryliuk pushed a commit that referenced this pull request Dec 9, 2021
Ran following commands:

```
brew install pngquant
pngquant --quality=65-80 ./packages/**/*.png --ext .png --force
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants