[bug fixes] annotations <> x domains, zeros in text

[williaster]

This PR fixes a couple bugs related to annotation usage

• when adding x-interval annotations to time series charts, the x-scale domain does not account for the annotation data. I updated the logic to account for this.
• when playing with annotations I noticed that you cannot currently enter a 0 in a TextControl component because the control only updates to values that evaluate truthy. I updated the logic to actually check for a value.

in examples below I added (interval or event) annotations with a table slice data source.

before (annotations clipped)

after (annotations included)

@mistercrunch @fabianmenges @graceguo-supercat @michellethomas

some other bugs I noted but didn't fix:

• if the source for event or interval annotations is a table, passing down "from" and "until" should actually perform a filter on the table columns defined by "interval start column" (and "interval end column"), not on the slices "from" and "until" form data.
• you can't set a y-max on a chart (looked into it, tricky with nvd3 without computing your own domain; I think we should fix with data-ui integration)

[mistercrunch]

@fabianmenges is the best person to review this

[fabianmenges]

I'll first comment on this:

if the source for event or interval annotations is a table, passing down "from" and "until" should actually perform a filter on the table columns defined by "interval start column" (and "interval end column"), not on the slices "from" and "until" form data.

I don't really think this is a bug but a use case that wasn't covered. The concepts of until and since are different from any other filter in Superset and the only ones I implemented.

That said, I thought about similar use cases before and there are some foundations you can use to do what you want to do.

Take a look here:
https://github.com/apache/incubator-superset/blob/04680e5ff138f7113a3d655133307049bc91ff3d/superset/assets/javascripts/explore/components/controls/AnnotationLayer.jsx#L408

This line will override the since field of the annotation slice to use the since value of the current slice. The logic that does that is here:

https://github.com/apache/incubator-superset/blob/04680e5ff138f7113a3d655133307049bc91ff3d/superset/assets/javascripts/chart/chartAction.js#L71

Now, in order to implement what you described you will need to add something like this to the AnnotationLayer:

this.setState({ overrides: { ...overrides, filters: [
  { col: timeColumn, op: '>', val: :  'since' },
  { col: intervalEndColumn, op: '<', val: :  'until' },
] } });

And make changes to the chartAction that appends/transforms the filters correctly. This will be pretty tricky to get working in a flexible way.

[fabianmenges]

I think its a bit weird that the filtering here has side effects. Might be cleaner to find the min and max values in that series after filtering.

[williaster]

👍

[fabianmenges]

The code currently does not really check that timeColumn < intervalEndColumn and will flip them around when necessary. Not sure if we need to handle that though.

[mistercrunch]

@williaster let's try to get this through, please resolve conflict and merge when you have a moment

[williaster]

@mistercrunch will try to get this done today

[williaster]

I think annotations are broken in production, so will debug that as part of this.

[williaster]

fixed annotations separately in #4607, updated this PR / should be good to go so PTAL 👀

[codecov-io]

Codecov Report

Merging #4194 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #4194      +/-   ##
==========================================
- Coverage   71.18%   71.18%   -0.01%     
==========================================
  Files         189      189              
  Lines       14847    14848       +1     
  Branches     1086     1085       -1     
==========================================
  Hits        10569    10569              
- Misses       4275     4276       +1     
  Partials        3        3

Impacted Files	Coverage Δ
...cripts/explore/components/controls/TextControl.jsx	45.45% <0%> (-1.43%)	⬇️

---

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 de44098...9ac4918. Read the comment docs.

[williaster]

@mistercrunch @fabianmenges going to merge this if no objections

[graceguo-supercat]

LGTM