fix(heatmap): snap time bucket to calendar/fixed intervals

[markov00]

Summary

The PR introduced the ability to specify both calendars and fixed intervals as described in Elasticserach date_histogram aggregation

This fix was required to cover the Lens usage when calendar intervals are used in conjunction with the date_histogram aggs.

BREAKING CHANGE

The xScaleType is replaced by the prop xScale, which better describes and collects the information related to the passed interval if any.

These are the exposed types as part of the API changes

interface HeatmapSpec extends Spec {
  ...
  xScale: RasterTimeScale | OrdinalScale | LinearScale;
  ...
}

export interface TimeScale {
  type: typeof ScaleType.Time;
}

export interface RasterTimeScale extends TimeScale {
  interval: ESCalendarInterval | ESFixedInterval;
}

export interface LinearScale {
  type: typeof ScaleType.Linear;
}

export interface OrdinalScale {
  type: typeof ScaleType.Ordinal;
}
interface ESFixedInterval {
  type: 'fixed';
  unit: ESFixedIntervalUnit;
  value: number;
}

interface ESCalendarInterval {
  type: 'calendar';
  unit: ESCalendarIntervalUnit;
  value: number;
}

type ESFixedIntervalUnit = 'ms' | 's' | 'm' | 'h' | 'd';

type ESCalendarIntervalUnit =
  | 'minute'
  | 'm'
  | 'hour'
  | 'h'
  | 'day'
  | 'd'
  | 'week'
  | 'w'
  | 'month'
  | 'M'
  | 'quarter'
  | 'q'
  | 'year'
  | 'y';

Details

The histogram chart needs to discretize the domain if a time scale is passed in the X axis. To correctly discretize based on the domain min, max values, we need to apply a specific rounding formula depending on timezone and type of interval (calendar/fixed). The discretization is required to cover the case of sparse datasets.

Issues

fix #1165

Checklist

• The proper chart type label has been added (e.g. :xy, :partition)
• The proper feature labels have been added (e.g. :interactions, :axis)
• All related issues have been linked (i.e. closes #123, fixes #123)
• New public API exports have been added to packages/charts/src/index.ts
• Unit tests have been added or updated to match the most common scenarios
• The proper documentation and/or storybook story has been added or updated

[nickofthyme]

changes LGTM

A side note for the future. I wonder if some of the ES types are available in @elastic/elasticsearch that we can leverage.

[monfera]

Not necessarily in this PR, we could eventually depend on types in elasticsearch.js as an authoritative source

[monfera]

Just to clarify, as we have an elasticsearch.ts, I meant, at some point directly devDepending on https://github.com/elastic/elasticsearch-js for these types. This way, if elasticsearch adds a new unit, and we bump the dependency, we'll get automatic static errors saying that we don't cover the new unit option(s)

[monfera]

Minor: A positively stated scale name would work better, if possible. The word "scalar" implies unitlessness but it doesn't look nice next to "Scale", so, maybe, UnitlessScale?

The word "Heatmap" in the scale name represents tight coupling, because it casts the Scale as a property of (subsidiary to) the chart type Heatmap, while they're at least equal, orthogonal things or even, Heatmap is subsidiary to the Scale. Of course I see how you want to rule out logarithmic etc. scales.

Maybe UnitlessHeatmapScale is an OK alternative

[markov00]

or I can completely remove that named type and use something like:

xScale:
    | HeatmapTimeScale
    | {
        type: typeof ScaleType.Ordinal | typeof ScaleType.Linear;
      };

wdyt?

with your consideration should also HeatmapTimeScale be renamed to TimeHeatmapScale?

[monfera]

Part 1: I'm fine with that too, though in terms of syntax, it combines two types of unioning, where it could be just one, eg. { type: ordinal | linear | time } or xScale: Heatmap | Ordinal | Linear

Part 2: Neither name is something I really prefer, as both of them tightly bind a scale type to a chart type maybe needlessly, though the first option rolls off the tongue more easily to me. But the best would be, if possible, to not have a chart type name in the name of a scale type. So, something like TimeScale, or however we call it in xy already

[monfera]

Nice! I hope TS will fix this oversight eventually. As it's currently an abstraction that's leaking from hundreds of self-inflicted wounds, Number.isFinite is one of those things that should narrow types (here, to number if the result is true), and this utility is a nice workaround in the meantime

[markov00]

I know, I was following your comments on the TS repo and I was thinking that, for now, can solve our cases

[monfera]

Maybe it could be a value: unknown, and it can be put into common or utils, as there are many places where I temporarily used a typeof d === 'number' && Number.isFinite(d) check where isFiniteNumber would be clearer. All places where we do === 'number' is an easy to grep candidate for a future PR. Even the name of this function is perfect 👌

[monfera]

LGTM! We discussed two improvements:

• long term: possible multilayer axis (after it had been extracted from xy)
• shorter term follow-up PR: avoiding tick overlap
  

[nickofthyme]

LGTM, I don't see any issues after playing with the new story for any of the datasets. Old stories work as before.

[nickofthyme]

🎉 This PR is included in version 39.0.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀