Generalize switch between different datasources #1078

ShengyaoQian · 2016-09-08T07:40:21Z

No description provided.

xrmx · 2016-09-08T07:59:08Z

caravel/migrations/versions/33d996bcc382_update_slice_model.py

+from caravel.utils import generic_find_constraint_name
+
+def find_constraint_name(col_names, referenced_table):
+	cols = col_names


nit: if you like cols better just rename the function argument

xrmx · 2016-09-08T08:01:42Z

Please state why you need these changes, what problem do they solve?

ShengyaoQian · 2016-09-08T18:31:33Z

Our team is adding a third kind of datasource, our backend api to caravel besides Druid and Sql. Currently in caravel, the switch between these two datasources are hard-coded, which requires us to do the same when we add our customized datasource engine. So we need to make changes inline to make things work. So we need to maintain our fork and deal with merges. Ideally, we want to keep our changes from upstream caravel as separated as possible. So it's easier to maintain and keep updated with upstream. I've created an issue before #1035, and was suggested to send a PR. So here's the changes I'm hoping to get in. I'm creating an instance of a registry that register all desired datasource type and Models at initialization of the app. So it'll pick up the Model associated with the datasource type in Slice Model and Views.py explore method. Let me know what you think.

joshwalters · 2016-09-08T22:06:42Z

@ShengyaoQian It looks like there are some checkstyle issues that have to be fixed: https://codeclimate.com/github/airbnb/caravel/pull/1078

Just to clarify, with this change you are trying to support the addition of 'generic' engines (not SQLAlchemy or Druid), correct?

How exactly will this pickup new datasource engines? I may have missed that in the code.

xrmx · 2016-09-09T07:32:19Z

@joshwalters he's adding a generic way to handle datasources. So that you don't need to touch every line of code that discriminate between sqla and druid.

@ShengyaoQian i think you need to update the add method of SliceModelView no?

xrmx · 2016-09-09T07:32:58Z

@ShengyaoQian may i ask which datasource are you adding?

mistercrunch · 2016-09-09T17:43:13Z

caravel/migrations/versions/33d996bcc382_update_slice_model.py

+from caravel.utils import generic_find_constraint_name
+
+def find_constraint_name(cols, referenced_table):
+	return generic_find_constraint_name(


the codebase is all 4 spaces indents, no tabs pls!

mistercrunch · 2016-09-09T17:47:32Z

I'm in favor of this PR but highlighted a few nits inline

joshwalters · 2016-09-09T20:04:00Z

caravel/utils.py

@@ -404,7 +404,11 @@ def readfile(filepath):
        content = f.read()
    return content

-
+def register_sources(models, registry):
+    print("Inside register_sources")


Should remove debug prints.

ShengyaoQian · 2016-09-09T22:19:44Z

@joshwalters I've made some changes in registering datasource. The modules and class will be passed in through config file. And be imported upon initialization of the app.

ShengyaoQian · 2016-09-09T22:22:59Z

@xrmx SliceModelView has been updated. Thanks for the notice. I'm adding our API as a third datasource engine. I take the query object generated by caravel and parse it into our desired format and make request to our api and then parse the json response into pandas df.

ps is "She" ;P

ShengyaoQian · 2016-09-09T22:31:16Z

I'm having difficulties making the db migration work for sqlite and postgres. So I'm dropping fk constraints between table, druiddatasources and slices. But when it was originally created, the fk doesn't have a name. So sqlite couldn't drop them. I referred to this file https://github.com/airbnb/caravel/blob/master/caravel/migrations/versions/1226819ee0e3_fix_wrong_constraint_on_table_columns.py where you seems to be dealing with the same issue. But I couldn't really get it to work with adding naming_convention. Any suggestions?

ShengyaoQian · 2016-09-09T22:56:08Z

tests/core_tests.py

-        )
-
-        db.session.delete(datasource)
-        db.session.commit()


Remove those test since the order of redirecting no longer applies.

xrmx · 2016-09-10T08:21:44Z

@ShengyaoQian @mistercrunch Do you think we can add the relevant sources to the datasource table when migrating so that we are not breaking every installation?

patelh · 2016-09-11T21:13:38Z

caravel/migrations/versions/33d996bcc382_update_slice_model.py

+            constraint_2 = find_constraint_name(cols_2, 'tables')
+            batch_op.drop_constraint(constraint_2, type_="foreignkey")
+
+            batch_op.drop_column('druid_datasource_id')


Maybe you don't want to drop these two columns first. Add the new column. Use the other two ids to populate datasource id. Meaning either druid_datasource_id or table_id would be non-null and you could set datasource_id to whichever one is not null. However, the downgrade would be difficult since you'd need to use the datasource type to determine which column to populate.

Changes are made accordingly.

ShengyaoQian · 2016-09-20T18:29:00Z

@mistercrunch ping!

mistercrunch · 2016-09-20T20:08:55Z

This is good to go, thanks for the PR!

mistercrunch · 2016-09-20T20:09:12Z

Ooops merge conflict, can't merge...

ShengyaoQian · 2016-09-21T05:54:52Z

@mistercrunch !

mistercrunch · 2016-09-21T16:52:02Z

Thanks for bearing through with us!

mistercrunch · 2016-09-21T16:54:07Z

ShengyaoQian · 2016-09-21T17:01:14Z

THANKS！ ~~^o^~~ Y(^_^)Y

the-dcruz · 2016-09-28T21:42:32Z

caravel/migrations/versions/27ae655e4247_make_creator_owners.py

-    objects = session.query(models.Slice).all()
-    objects += session.query(models.Dashboard).all()
+    objects = session.query(Slice).all()
+    objects += session.query(Dashboard).all()


created_by does not exist as a column in the slice or dashboard tables causing an exception to be raised two lines below this if objects is not empty.

Does it make sense to modify existing migration files? Existing environments would not rerun this script right?

@the-dcruz could you please open a new issue with the stacktrace please?

@the-dcruz I added a column datasource_id in Slice and Dashboard. If it uses models.Slice and models.Dashboard the session.query would fail because it couldn't find this column at this migration point. It would be helpful if you can share the stacktrace. Thank you

@the-dcruz I run into some problem as well and I think it's what you have. I think the tests run on empty databases that's why I didn't catch it the first time. When there's existing data, you'd see the error. I've created a pull request for the fix. Can you take a look to see if it fixes what you have? #1262

ShengyaoQian force-pushed the src_reg branch from 2ef6f52 to 8eb5292 Compare September 8, 2016 07:41

ShengyaoQian changed the title ~~Src reg~~ Generalize switch between different datasources Sep 8, 2016

ShengyaoQian mentioned this pull request Sep 8, 2016

Rendering Visualizations for customized engines #1035

Closed

xrmx reviewed Sep 8, 2016
View reviewed changes

mistercrunch reviewed Sep 9, 2016
View reviewed changes

joshwalters reviewed Sep 9, 2016
View reviewed changes

ShengyaoQian reviewed Sep 9, 2016
View reviewed changes

patelh reviewed Sep 11, 2016
View reviewed changes

Shengyao Qian added 8 commits September 12, 2016 09:49

Generalize switch between different datasources.

d73e9a0

Fix previous migration since slice model changed

383ffe4

Fix warm up cache and other small stuff

22caae3

Adding modules and datasources through config

44d0e6f

Replace tabs w/ spaces

81ffb89

Fix other style issues

1f60b8d

Change add method for SliceModelView to pick the first non-empty ds

9d82efa

Remove tests on slice add redirect

b96628f

Remove all getter methods

d4b2976

ShengyaoQian force-pushed the src_reg branch from c54dc44 to d4b2976 Compare September 20, 2016 17:47

Merge branch 'master' into src_reg

05e80ef

Shengyao Qian and others added 2 commits September 20, 2016 13:56

Merge branch 'master' into src_reg

cad85ae

Merge branch 'master' into src_reg

a386f60

ShengyaoQian force-pushed the src_reg branch from 4cc8f63 to a386f60 Compare September 20, 2016 23:14

Merge branch 'master' into src_reg

0e4f221

xrmx mentioned this pull request Sep 21, 2016

Better error message when removing used druid datasource #1141

Closed

mistercrunch merged commit 5a0e06e into apache:master Sep 21, 2016

the-dcruz reviewed Sep 28, 2016

View reviewed changes

ShengyaoQian mentioned this pull request Oct 5, 2016

Fix migration for make creator owners #1262

Merged

This was referenced Jun 2, 2021

[Snyk] Upgrade @superset-ui/plugin-chart-table from 0.15.13 to 0.17.44 lili-test/incubator-superset#3

Open

[Snyk] Upgrade @superset-ui/legacy-preset-chart-nvd3 from 0.15.14 to 0.17.42 lili-test/incubator-superset#5

Open

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021

feat(plugin-chart-table): disable show_totals by default (apache#1078)

3c209e3

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021

feat(plugin-chart-table): disable show_totals by default (apache#1078)

fba2b74

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021

feat(plugin-chart-table): disable show_totals by default (apache#1078)

4d9a45d

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021

feat(plugin-chart-table): disable show_totals by default (apache#1078)

08a0d0a

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize switch between different datasources #1078

Generalize switch between different datasources #1078

ShengyaoQian commented Sep 8, 2016

xrmx Sep 8, 2016

xrmx commented Sep 8, 2016

ShengyaoQian commented Sep 8, 2016 •

edited

Loading

joshwalters commented Sep 8, 2016 •

edited

Loading

xrmx commented Sep 9, 2016

xrmx commented Sep 9, 2016

mistercrunch Sep 9, 2016

mistercrunch commented Sep 9, 2016

joshwalters Sep 9, 2016

ShengyaoQian commented Sep 9, 2016

ShengyaoQian commented Sep 9, 2016

ShengyaoQian commented Sep 9, 2016

ShengyaoQian Sep 9, 2016

xrmx commented Sep 10, 2016

patelh Sep 11, 2016

ShengyaoQian Sep 13, 2016

ShengyaoQian commented Sep 20, 2016

mistercrunch commented Sep 20, 2016

mistercrunch commented Sep 20, 2016

ShengyaoQian commented Sep 21, 2016

mistercrunch commented Sep 21, 2016

mistercrunch commented Sep 21, 2016

ShengyaoQian commented Sep 21, 2016

the-dcruz Sep 28, 2016

the-dcruz Sep 28, 2016

xrmx Sep 29, 2016

ShengyaoQian Sep 29, 2016

ShengyaoQian Oct 5, 2016

Generalize switch between different datasources #1078

Generalize switch between different datasources #1078

Conversation

ShengyaoQian commented Sep 8, 2016

Choose a reason for hiding this comment

xrmx commented Sep 8, 2016

ShengyaoQian commented Sep 8, 2016 • edited Loading

joshwalters commented Sep 8, 2016 • edited Loading

xrmx commented Sep 9, 2016

xrmx commented Sep 9, 2016

Choose a reason for hiding this comment

mistercrunch commented Sep 9, 2016

Choose a reason for hiding this comment

ShengyaoQian commented Sep 9, 2016

ShengyaoQian commented Sep 9, 2016

ShengyaoQian commented Sep 9, 2016

Choose a reason for hiding this comment

xrmx commented Sep 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShengyaoQian commented Sep 20, 2016

mistercrunch commented Sep 20, 2016

mistercrunch commented Sep 20, 2016

ShengyaoQian commented Sep 21, 2016

mistercrunch commented Sep 21, 2016

mistercrunch commented Sep 21, 2016

ShengyaoQian commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShengyaoQian commented Sep 8, 2016 •

edited

Loading

joshwalters commented Sep 8, 2016 •

edited

Loading