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: heatmap snap domain to interval #1253

Merged
merged 13 commits into from
Jul 30, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ import { ScaleContinuous } from '../../../../scales';
import { ScaleType } from '../../../../scales/constants';
import { SettingsSpec } from '../../../../specs';
import { CanvasTextBBoxCalculator } from '../../../../utils/bbox/canvas_text_bbox_calculator';
import { clamp } from '../../../../utils/common';
import { snapDateToESInterval } from '../../../../utils/chrono/elasticsearch';
import { clamp, range } from '../../../../utils/common';
import { Dimensions } from '../../../../utils/dimensions';
import { PrimitiveValue } from '../../../partition_chart/layout/utils/group_by_rollup';
import { HeatmapSpec } from '../../specs';
Expand Down Expand Up @@ -101,9 +102,6 @@ export function shapeViewModel(

const yInvertedScale = scaleQuantize<NonNullable<PrimitiveValue>>().domain([0, height]).range(yValues);

// TODO: Fix domain type to be `Array<number | string>`
let xValues = xDomain.domain as any[];

const timeScale =
xDomain.type === ScaleType.Time
? new ScaleContinuous(
Expand All @@ -120,17 +118,17 @@ export function shapeViewModel(
)
: null;

if (timeScale) {
const result = [];
let [timePoint] = xValues;
while (timePoint < xValues[1]) {
result.push(timePoint);
timePoint += xDomain.minInterval;
}

xValues = result;
}

const xValues = timeScale
? range(
snapDateToESInterval(
xDomain.domain[0] as number,
{ type: 'fixed', unit: 'ms', quantity: xDomain.minInterval },
'start',
),
xDomain.domain[1] as number,
xDomain.minInterval,
)
: xDomain.domain;
// compute the scale for the columns positions
const xScale = scaleBand<NonNullable<PrimitiveValue>>().domain(xValues).range([0, chartDimensions.width]);

Expand Down Expand Up @@ -298,8 +296,12 @@ export function shapeViewModel(
const endValue = x[x.length - 1];

// find X coordinated based on the time range
const leftIndex = typeof startValue === 'number' ? bisectLeft(xValues, startValue) : xValues.indexOf(startValue);
const rightIndex = typeof endValue === 'number' ? bisectLeft(xValues, endValue) : xValues.indexOf(endValue) + 1;
// the xValues array is casted as any because the slight incompatible type of d3 bisect.
// it works anyway without problems also if the data is a mix of string and numbers
const leftIndex =
typeof startValue === 'number' ? bisectLeft(xValues as any, startValue) : xValues.indexOf(startValue);
const rightIndex =
typeof endValue === 'number' ? bisectLeft(xValues as any, endValue) : xValues.indexOf(endValue) + 1;
Copy link
Contributor

@monfera monfera Jul 27, 2021

Choose a reason for hiding this comment

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

The type of the 1st and 2nd params of bisectLeft need to match, and we're typeguarding the 2nd param to number, so it's slightly tighter to say as number[]. Though it's not truthful, maybe we could brainstorm so I can learn about whether OrdinalDomain needs to be (number | string)[] and can't be number[] | string[] eg. by coercing numbers to strings if at least one element is a string. Though there's still the lack of type coherence between xValues and startValue - probably they correlate strongly (otherwise we can't do as any or as number[] here) but this seems to be lost on TS

Copy link
Contributor

Choose a reason for hiding this comment

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

If the type of bisectLeft is incorrect or too restrictive (we can't overrule it as it's beyond what the D3 TS API guarantees, but iirc these are 3rd party post hoc libs, not canonical Bostock contract) then maybe it'd be better to factor it out into our bisectLeft utility with the desired types, so we can easily change from D3 if needed. I think we could already use the also logarithmically bisecting monotonicHillClimb instead of d3.bisectLeft

Copy link
Member Author

Choose a reason for hiding this comment

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

There is one main reason for having (number | string)[] as OrdinalDomain: we like to preserve the original type, coming from the user data, to return the original values on the interaction callbacks (brushing or clicking)
We can probably force it to be number[] | string[] but I don't have strong opinion here

Copy link
Contributor

@monfera monfera Jul 27, 2021

Choose a reason for hiding this comment

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

Some more info may strengthen the case for string[] | number[]:

D3 bisectLeft, and generally, logarithmic search requires total order, which we likely ensure by sorting the values beforehand(*).

Out of the properties of total order, transitivity is most important: if a <= b and b <= c then a <= c must be true (it's not necessarily true for partial orders, eg. when a number is defined non-comparable to a string).

(*) The mechanism doesn't matter, eg. [].sort also relies on a predicate plus array which together guarantee a total order, otherwise the order is ill defined: EcmaScript requires transitivity for predictable results: If a <CF b and b <CF c, then a <CF c (transitivity of <CF).

Sorting (number | string)[] with a < predicate doesn't lead to total order. Example:

a = '2'
b = 2.5
c = '10'

a < b: true
b < c: true
a < c: false

So a sensible option is to string-compare all elements if at least one element in the array is not a number, which is the default sort predicate (and number-compare for all-numeric arrays, which is not the default sort predicate). Instead of checking for this at the place of sorting, we may as well come clean and convert upfront. As you suggested Marco, the original value can still be retained. The effect of the conversion though is that a user-supplied [5, '5'] will map to ['5', '5'] so an equally good method is, documenting in the API, perhaps even enforcing via TS, that the domain values be homogeneous.

There can of course be other sorting rules, eg. all strings are moved to the end (tiebreaker rule for when a string and a number are compared), and within both the strings and numbers, there's full order


const isRightOutOfRange = rightIndex > xValues.length - 1 || rightIndex < 0;
const isLeftOutOfRange = leftIndex > xValues.length - 1 || leftIndex < 0;
Expand Down
89 changes: 89 additions & 0 deletions packages/charts/src/utils/chrono/chrono.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

// NOTE: to switch implementation just change the imported file (moment,luxon)
import {
addTimeToObj,
timeObjToUnixTimestamp,
startTimeOfObj,
endTimeOfObj,
timeObjFromAny,
timeObjToUTCOffset,
subtractTimeToObj,
formatTimeObj,
diffTimeObjs,
} from './moment';
Comment on lines +9 to +20
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really not sure why this is needed as this is not configurable from outside of charts.

As it is we use moment across the charts code so are you thinking of being able to swap time libraries entirely? In that case we could have optional dependencies on moment and luxon so the user can switch between them.

If not is this to only be used as a utility to handle different time types?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was done in the hope we can switch soon and easily to luxon or to Temporal https://tc39.es/proposal-temporal/docs/index.html
It really depends on Kibana right now, so extracting everything like that can help us migrating easily

import { CalendarIntervalUnit, CalendarObj, DateTime, FixedIntervalUnit, Minutes, UnixTimestamp } from './types';

/** @internal */
export function addTime(
dateTime: DateTime,
timeZone: string | undefined,
unit: keyof CalendarObj,
count: number,
): UnixTimestamp {
return timeObjToUnixTimestamp(addTimeToObj(getTimeObj(dateTime, timeZone), unit, count));
}

/** @internal */
export function subtractTime(
dateTime: DateTime,
timeZone: string | undefined,
unit: keyof CalendarObj,
count: number,
): UnixTimestamp {
return timeObjToUnixTimestamp(subtractTimeToObj(getTimeObj(dateTime, timeZone), unit, count));
}

/** @internal */
export function getUnixTimestamp(dateTime: DateTime, timeZone?: string): UnixTimestamp {
return timeObjToUnixTimestamp(getTimeObj(dateTime, timeZone));
}

/** @internal */
export function startOf(
dateTime: DateTime,
timeZone: string | undefined,
unit: CalendarIntervalUnit | FixedIntervalUnit,
): UnixTimestamp {
return timeObjToUnixTimestamp(startTimeOfObj(getTimeObj(dateTime, timeZone), unit));
}

/** @internal */
export function endOf(
dateTime: DateTime,
timeZone: string | undefined,
unit: CalendarIntervalUnit | FixedIntervalUnit,
): UnixTimestamp {
return timeObjToUnixTimestamp(endTimeOfObj(getTimeObj(dateTime, timeZone), unit));
}

function getTimeObj(dateTime: DateTime, timeZone?: string) {
return timeObjFromAny(dateTime, timeZone);
}

/** @internal */
export function getUTCOffset(dateTime: DateTime, timeZone?: string): Minutes {
return timeObjToUTCOffset(getTimeObj(dateTime, timeZone));
}

/** @internal */
export function formatTime(dateTime: DateTime, timeZone: string | undefined, format: string) {
return formatTimeObj(getTimeObj(dateTime, timeZone), format);
}

/** @internal */
export function diff(
dateTime1: DateTime,
timeZone1: string | undefined,
dateTime2: DateTime,
timeZone2: string | undefined,
unit: CalendarIntervalUnit | FixedIntervalUnit,
) {
return diffTimeObjs(getTimeObj(dateTime1, timeZone1), getTimeObj(dateTime2, timeZone2), unit);
}
57 changes: 57 additions & 0 deletions packages/charts/src/utils/chrono/elasticsearch.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { DateTime } from 'luxon';

import { snapDateToESInterval } from './elasticsearch';

describe('snap to interval', () => {
it('should snap to begin of calendar interval', () => {
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z');
const snappedDate = snapDateToESInterval(
initialDate.toMillis(),
{ type: 'calendar', unit: 'd', quantity: 1 },
'start',
'UTC',
);
expect(DateTime.fromMillis(snappedDate, { zone: 'utc' }).toISO()).toBe('2020-01-03T00:00:00.000Z');
});

it('should snap to end of calendar interval', () => {
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z');
const snappedDate = snapDateToESInterval(
initialDate.toMillis(),
{ type: 'calendar', unit: 'd', quantity: 1 },
'end',
'UTC',
);
expect(DateTime.fromMillis(snappedDate, { zone: 'utc' }).toISO()).toBe('2020-01-03T23:59:59.999Z');
});

it('should snap to begin of fixed interval', () => {
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z');
const snappedDate = snapDateToESInterval(
initialDate.toMillis(),
{ type: 'fixed', unit: 'm', quantity: 30 },
'start',
'UTC',
);
expect(DateTime.fromMillis(snappedDate, { zone: 'utc' }).toISO()).toBe('2020-01-03T07:00:00.000Z');
});

it('should snap to end of fixed interval', () => {
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z');
const snappedDate = snapDateToESInterval(
initialDate.toMillis(),
{ type: 'fixed', unit: 'm', quantity: 30 },
'end',
'UTC',
);
expect(DateTime.fromMillis(snappedDate, { zone: 'utc' }).toISO()).toBe('2020-01-03T07:29:59.999Z');
});
});
119 changes: 119 additions & 0 deletions packages/charts/src/utils/chrono/elasticsearch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { TimeMs } from '../../common/geometry';
import { endOf, getUnixTimestamp, startOf } from './chrono';
import { CalendarIntervalUnit, FixedIntervalUnit, UnixTimestamp } from './types';

/** @internal */
export type ESCalendarIntervalUnit =
| 'minute'
| 'm'
| 'hour'
| 'h'
| 'day'
| 'd'
| 'week'
| 'w'
| 'month'
| 'M'
| 'quarter'
| 'q'
| 'year'
| 'y';
monfera marked this conversation as resolved.
Show resolved Hide resolved

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

/** @internal */
export const ES_FIXED_INTERVAL_UNIT_TO_BASE: Record<ESFixedIntervalUnit, TimeMs> = {
ms: 1,
s: 1000,
m: 1000 * 60,
h: 1000 * 60 * 60,
d: 1000 * 60 * 60 * 24,
};

/** @internal */
export type ESCalendarInterval = {
type: 'calendar';
unit: ESCalendarIntervalUnit;
quantity: number;
};

/** @internal */
export interface ESFixedInterval {
type: 'fixed';
unit: ESFixedIntervalUnit;
quantity: number;
}

/** @internal */
export function snapDateToESInterval(
date: number | Date,
interval: ESCalendarInterval | ESFixedInterval,
snapTo: 'start' | 'end',
timeZone?: string,
): UnixTimestamp {
return isCalendarInterval(interval)
? esCalendarIntervalSnap(date, interval, snapTo, timeZone)
: esFixedIntervalSnap(date, interval, snapTo, timeZone);
}

function isCalendarInterval(interval: ESCalendarInterval | ESFixedInterval): interval is ESCalendarInterval {
return interval.type === 'calendar';
}

function esCalendarIntervalSnap(
date: number | Date,
interval: ESCalendarInterval,
snapTo: 'start' | 'end',
timeZone?: string,
) {
return snapTo === 'start'
? startOf(date, timeZone, esCalendarIntervalToChronoInterval(interval.unit))
: endOf(date, timeZone, esCalendarIntervalToChronoInterval(interval.unit));
}
function esFixedIntervalSnap(
date: number | Date,
interval: ESFixedInterval,
snapTo: 'start' | 'end',
timeZone?: string,
): UnixTimestamp {
const unitMultiplier = interval.quantity * ES_FIXED_INTERVAL_UNIT_TO_BASE[interval.unit];
const unixTimestamp = getUnixTimestamp(date, timeZone);
const roundedDate = Math.floor(unixTimestamp / unitMultiplier) * unitMultiplier;
return snapTo === 'start' ? roundedDate : roundedDate + unitMultiplier - 1;
}

function esCalendarIntervalToChronoInterval(unit: ESCalendarIntervalUnit): CalendarIntervalUnit | FixedIntervalUnit {
switch (unit) {
case 'minute':
case 'm':
return 'minute';
case 'hour':
case 'h':
return 'hour';
case 'day':
case 'd':
return 'day';
case 'week':
case 'w':
return 'week';
case 'month':
case 'M':
return 'month';
case 'quarter':
case 'q':
return 'quarter';
case 'year':
case 'y':
return 'year';
default:
return 'hour';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit

Suggested change
default:
return 'hour';
}
case 'hour':
case 'h':
default:
return 'hour';
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I've update the logic to use a map instead of a switch as proposed by robert

}
Loading