Interval annotations on charts broken in master

martyngigg commented 1 week ago

Bug description

Annotation layers of type "Interval" no longer seem to appear on line charts in master. They are fine in 4.02-dev and 4.1.0rc2-dev but not on master.

It does not matter if the source is a 'Superset annotation' or a 'Table'.

How to reproduce the bug

This uses the examples datasets.

Spin up the latest development image: TAG=994de1f-dev docker compose -f ./docker-compose-image-tag.yml up -d
Go to 'Settings -> Annotation Layers'.
- Click + Annotation layer button
- Give the layer a name
- Click Add
Click on the name of the new annotation layer
- Click + Annotation button
- Give the annotation a name
- Set the start date to 2000-01-01 00:00:00 and end date to 2004-01-01 00:00:00
- Click Add
Click Charts in the navigation bar
- Click + Chart
- Select the birth_names dataset
- Select the Line Chart type
- Click Create new chart
Configure the chart
- Drag the ds column to X-axis
- Drag the num column to Metrics, choose the Sum aggregate and click Save
- Scroll down in the configure panel and click Annotations and Layers followed by + Add annotation layer
- Give the layer a name
- Check the Show label box
- Change the Annotation layer type to Interval
- Change the Annotation source to Superset annotation
- Change the Annotation layer to the name given to the newly created layer in step 2
- Click OK
- Click Update chart

I cannot see any errors in the superset logs. I've attached a screenshot of an error in the Firefox console.

Screenshots/recordings

Superset-Missing-Annotations

Superset version

master / latest-dev

Python version

3.10

Node version

18 or greater

Browser

Firefox

Additional context

No response

Checklist

[X] I have searched Superset docs and Slack and didn't find a solution to my problem.
[X] I have searched the GitHub issue tracker and didn't find a similar bug report.
[X] I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.

dosubot[bot] commented 1 week ago

@martyngigg Hello! I'm here to help you with any bugs, questions, or contributions. Let's get this sorted out together!

I found a similar closed issue: Line Chart annotations don't load on Time-Series Line Charts. The issue was fixed on the master branch and will be included in the upcoming 2.1.0 and/or 3.0 releases. If needed, the fix can be isolated to a single PR and applied to version 2.0.2 by making a request on the specified thread ^[1].

The interval annotations on line charts are not appearing in the master version of Apache Superset because the code responsible for updating the annotation positions on brush events is missing in the master version. This code is present in the 4.02-dev and 4.1.0rc2-dev versions. The following snippet is responsible for updating the interval annotations on brush events:

// update annotation positions on brush event
if (chart.focus) {
  chart.focus.dispatch.on('onBrush.interval-annotation', () => {
    annotations
      .selectAll('rect')
      .data(records)
      .attr({
        x: d => xScale(new Date(d[e.timeColumn])),
        width: d => {
          const x1 = xScale(new Date(d[e.timeColumn]));
          const x2 = xScale(new Date(d[e.intervalEndColumn]));
          return Math.max(Math.abs(x2 - x1), 1);
        },
      });
  });
}

Additionally, the transformProps function in the Timeseries plugin for Echarts in Apache Superset is responsible for transforming the chart properties, including annotations. The test cases for transformProps show that interval annotations are expected to be included in the transformed properties. If the interval annotations are not appearing, it could be due to a missing or incorrect implementation in the transformProps function in the master version ^[2]^[3]^[4].

To continue talking to Dosu, mention @dosu.

kgabryje commented 1 week ago

@martyngigg https://github.com/apache/superset/pull/30348 should have fixed the bug. Closing this issue, feel free to reopen if the bug persists

martyngigg commented 5 days ago

@kgabryje Yep looks good in my example now. Thanks!

apache / superset