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

Fix annotation_json endpoint #5621

Merged
merged 8 commits into from
Aug 14, 2018
Merged

Fix annotation_json endpoint #5621

merged 8 commits into from
Aug 14, 2018

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Aug 13, 2018

Current implementation of /annotations_json/{layerId} is broken. Simplified the endpoint to filter based upon the layer.

@betodealmeida @mistercrunch

@hughhhh hughhhh added the lyft Related to Lyft label Aug 13, 2018
@@ -47,8 +47,6 @@ def query(self, query_obj):
error_message = None
qry = db.session.query(Annotation)
qry = qry.filter(Annotation.layer_id == query_obj['filter'][0]['val'])
qry = qry.filter(Annotation.start_dttm >= query_obj['from_dttm'])
qry = qry.filter(Annotation.end_dttm <= query_obj['to_dttm'])
Copy link
Member

Choose a reason for hiding this comment

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

@hughhhh, why do you need to remove this? I'm worried it might bring too many annotations to the browser if we remove this — it would bring all holidays, instead of only the ones being displayed, eg.

Copy link
Member

Choose a reason for hiding this comment

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

how about:

if query_obj['from_dttm']:
    qry = qry.filter(Annotation.start_dttm >= query_obj['from_dttm'])

Copy link
Member Author

Choose a reason for hiding this comment

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

looking at the annotations_json endpoint we don't set the from_dttm or to_dttm at all. I just wanted to fix this endpoint to actually work because on master its not working at all.

https://github.com/apache/incubator-superset/blob/master/superset/views/core.py#L1118

superset/viz.py Outdated
df = db_engine_spec.adjust_df_column_names(df, self.form_data)
if self.status != utils.QueryStatus.FAILED:
stats_logger.incr('loaded_from_source')
is_loaded = True
Copy link
Member

Choose a reason for hiding this comment

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

Same here: why is this broken? I'm worried changing this will break existing code.

Copy link
Member

Choose a reason for hiding this comment

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

Removing this looks dangerous, the hasattr condition seems to be for preventing druid-related exception (a condition on that would be more explicit here). I'm pretty sure it's there for a reason.

@villebro
Copy link
Member

I added that. It's there to check whether the datasource is sqla or not. Apparently the check needs to be for datasource.database, not datasource.database.db_engine_spec.

@@ -47,7 +47,8 @@ def query(self, query_obj):
error_message = None
qry = db.session.query(Annotation)
qry = qry.filter(Annotation.layer_id == query_obj['filter'][0]['val'])
qry = qry.filter(Annotation.start_dttm >= query_obj['from_dttm'])
if query_obj['from_dttm']:
qry = qry.filter(Annotation.start_dttm >= query_obj['from_dttm'])
Copy link
Member

Choose a reason for hiding this comment

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

oh we need to same for to_dttm I think

Copy link
Member Author

Choose a reason for hiding this comment

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

to_dttm is set to the current date in the query object. I can add the guard though 👍

@villebro
Copy link
Member

@hughhhh To add some context, db_engine_spec.adjust_df_column_names(df, self.form_data) is supposed to be called for sqla datasources. Since Druid doesn't have a db_engine_spec but does have a database attribute, I assumed that I only needed to do check if database has a db_engine_spec attribute. Changing it to if hasattr(self.datasource, 'database') and hasattr(self.datasource.database, 'db_engine_spec'): probably should do the trick.

@hughhhh
Copy link
Member Author

hughhhh commented Aug 14, 2018

@villebro I think the check is fine. The problem is that the AnnotationsDatasource object is missing fields and is basically buffer to leverage the viz.py file. I set the database field to None for now so bypass this condition.

@hughhhh
Copy link
Member Author

hughhhh commented Aug 14, 2018

I'll go with your approach

@codecov-io
Copy link

codecov-io commented Aug 14, 2018

Codecov Report

Merging #5621 into master will increase coverage by 0.24%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5621      +/-   ##
==========================================
+ Coverage   63.48%   63.73%   +0.24%     
==========================================
  Files         360      360              
  Lines       22886    23113     +227     
  Branches     2547     2548       +1     
==========================================
+ Hits        14529    14730     +201     
- Misses       8342     8368      +26     
  Partials       15       15
Impacted Files Coverage Δ
superset/connectors/sqla/models.py 78.66% <0%> (-0.31%) ⬇️
superset/viz.py 81.16% <100%> (-0.02%) ⬇️
superset/models/core.py 85.09% <0%> (-0.1%) ⬇️
superset/assets/src/explore/visTypes.jsx 57.14% <0%> (ø) ⬆️
superset/assets/src/visualizations/line_multi.js 0% <0%> (ø) ⬆️
superset/assets/src/modules/colors.js 77.55% <0%> (ø) ⬆️
superset/assets/src/explore/controls.jsx 45.92% <0%> (ø) ⬆️
...uperset/assets/src/SqlLab/components/SqlEditor.jsx 78.09% <0%> (ø) ⬆️
superset/connectors/druid/models.py 82.56% <0%> (+0.02%) ⬆️
... and 2 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 536478e...bd320d8. Read the comment docs.

@mistercrunch
Copy link
Member

LGTM

@hughhhh hughhhh merged commit 85a6da1 into apache:master Aug 14, 2018
hughhhh added a commit to lyft/incubator-superset that referenced this pull request Aug 14, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Aug 16, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Aug 17, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
hughhhh added a commit to lyft/incubator-superset that referenced this pull request Aug 17, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Aug 18, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Aug 20, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Aug 20, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Aug 21, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Aug 21, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Aug 21, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
hughhhh added a commit to lyft/incubator-superset that referenced this pull request Aug 22, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Aug 22, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Aug 22, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Aug 22, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting

(cherry picked from commit 85a6da1)
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* fix annotation_json endpoint

* add time back

* set db to None

* change if condition

* remove prop

* add guard for to_dttm

* linting
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 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 lyft Related to Lyft 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants