Skip to content

Commit

Permalink
Bug/timeattribute (#823)
Browse files Browse the repository at this point in the history 
* Add helpers for getting time dimension from DataCube. Now timeAttribute is a required property.

* Use real time dimension and its reference instead of relying on the fact that name of time attribute is equal to name of time dimension.

* TimeAttribute is required so we always print it.

* More strict checks for Cluster object

* Strict checks around timeAttribute

* TimeAttribute is attribute name so use it to find dimension first and then use dimension reference in fresh default filter

* Add better warnings for missing timeAttribute property
  • Loading branch information
@adrianmroz-allegro
adrianmroz-allegro authored Feb 2, 2022
1 parent d3565d1 commit b809af8
Show file tree
Hide file tree
Showing 7 changed files with 176 additions and 51 deletions.
3 changes: 2 additions & 1 deletion src/client/components/filter-tile/filter-tiles-row.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

import * as React from "react";
import { getTimeDimensionReference } from "../../../common/models/data-cube/data-cube";
import { Dimension } from "../../../common/models/dimension/dimension";
import { findDimensionByName } from "../../../common/models/dimension/dimensions";
import { DragPosition } from "../../../common/models/drag-position/drag-position";
Expand Down Expand Up @@ -166,7 +167,7 @@ export class FilterTilesRow extends React.Component<FilterTilesRowProps, FilterT
let tryingToReplaceTime = false;
if (position.isReplace()) {
const targetClause = filter.clauses.get(position.replace);
tryingToReplaceTime = targetClause && targetClause.reference === dataCube.timeAttribute;
tryingToReplaceTime = targetClause && targetClause.reference === getTimeDimensionReference(dataCube);
if (tryingToReplaceTime) return;
}
addPartialFilter(dimension, position);
Expand Down
116 changes: 102 additions & 14 deletions src/common/models/data-cube/data-cube.mocha.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,20 @@
* limitations under the License.
*/

import { expect } from "chai";
import { expect, use } from "chai";
import { $, AttributeInfo } from "plywood";
import { SinonSpy, spy } from "sinon";
import equivalent from "../../../client/utils/test-utils/equivalent";
import { deduceAttributes } from "../../utils/external/datacube-to-external";
import { Cluster } from "../cluster/cluster";
import { createDimension } from "../dimension/dimension";
import { allDimensions } from "../dimension/dimensions";
import { DataCube, fromConfig } from "./data-cube";
import { createDimension, DimensionJS, timeDimension } from "../dimension/dimension";
import { fromConfig as dimensionsFromConfig } from "../dimension/dimensions";
import { allDimensions, findDimensionByExpression } from "../dimension/dimensions";
import { DataCube, DataCubeJS, fromConfig } from "./data-cube";
import { addAttributes } from "./queryable-data-cube";

use(equivalent);

describe("DataCube", () => {
const druidCluster = Cluster.fromJS({
name: "druid"
Expand Down Expand Up @@ -53,7 +58,7 @@ describe("DataCube", () => {
formula: "$main.sum($count)"
}
]
});
}, druidCluster);
}).to.throw("'wiki hello' is not a URL safe name. Try 'wiki_hello' instead?");
});

Expand Down Expand Up @@ -81,7 +86,7 @@ describe("DataCube", () => {
formula: "$main.sum($count)"
}
]
});
}, druidCluster);
}).to.throw("Can not find defaultSortMeasure 'gaga' in data cube 'wiki'");
});

Expand All @@ -108,7 +113,7 @@ describe("DataCube", () => {
formula: "$main.sum($count)"
}
]
});
}, druidCluster);
}).to.throw("data cube: 'wiki', names: 'articleName' found in both dimensions and measures");
});

Expand Down Expand Up @@ -139,7 +144,7 @@ describe("DataCube", () => {
formula: "$articleName"
}
]
});
}, druidCluster);
}).to.throw("data cube: 'wiki', found duplicate measure with name: 'articleName'");
});

Expand Down Expand Up @@ -170,7 +175,7 @@ describe("DataCube", () => {
formula: "$main.sum($count)"
}
]
});
}, druidCluster);
}).to.throw("data cube: 'wiki', found duplicate dimension with name: 'articleName'");
});

Expand Down Expand Up @@ -405,7 +410,7 @@ describe("DataCube", () => {
refreshRule: {
rule: "realtime"
}
});
}, druidCluster);
/* TODO: check the correctness of the test */
/*
it("works in basic case (no count) + re-add", () => {
Expand Down Expand Up @@ -656,7 +661,7 @@ describe("DataCube", () => {
formula: "${added!!!}"
}
]
});
}, druidCluster);

const dataCube = addAttributes(dataCubeWithDim, attributes1);
expect(Object.keys(dataCube.measures.byName)).to.deep.equal(["deleted"]);
Expand All @@ -676,7 +681,7 @@ describe("DataCube", () => {
refreshRule: {
rule: "realtime"
}
});
}, druidCluster);

it("adds new dimensions", () => {
const columns: any = [
Expand All @@ -691,20 +696,103 @@ describe("DataCube", () => {
const dataCube1 = addAttributes(dataCube, AttributeInfo.fromJSs(columns));

expect(allDimensions(dataCube1.dimensions)).to.deep.equal([
createDimension("time", "__time", $("__time")),
createDimension("time", "time", $("__time")),
createDimension("string", "page", $("page"))
]);

columns.push({ name: "channel", type: "STRING" });
const dataCube2 = addAttributes(dataCube1, AttributeInfo.fromJSs(columns));

expect(allDimensions(dataCube2.dimensions)).to.deep.equal([
createDimension("time", "__time", $("__time")),
createDimension("time", "time", $("__time")),
createDimension("string", "page", $("page")),
createDimension("string", "channel", $("channel"))
]);
});
});

describe("timeAttribute", () => {
describe("Druid clusters", () => {
const baseCube: DataCubeJS = {
name: "wiki",
clusterName: "druid",
source: "wiki"
};

const timeDimensionJS: DimensionJS = {
name: "time",
kind: "time",
formula: "$__time"
};

describe("timeAttribute property warnings", () => {
let consoleWarnSpy: SinonSpy;

beforeEach(() => {
consoleWarnSpy = spy(console, "warn");
});

afterEach(() => {
consoleWarnSpy.restore();
});

it("should warn if timeAttribute is missing", () => {
fromConfig({ ...baseCube }, druidCluster);
expect(consoleWarnSpy.args[0][0]).to.be.equal("DataCube \"wiki\" should have property timeAttribute. Setting timeAttribute to default value \"__time\"");
});

it("should warn if timeAttribute has different value than \"__time\"", () => {
fromConfig({ ...baseCube, timeAttribute: "foobar" }, druidCluster);
expect(consoleWarnSpy.args[0][0]).to.be.equal('timeAttribute in DataCube "wiki" should have value "__time" because it is required by Druid. Overriding timeAttribute to "__time"');
});
});

it("should add timeAttribute", () => {
const cube = fromConfig({ ...baseCube, dimensions: [timeDimensionJS] }, druidCluster);
expect(cube.timeAttribute).to.be.equivalent($("__time"));
});

it("should prepend time dimension if not defined", () => {
const cube = fromConfig({ ...baseCube, dimensions: [] }, druidCluster);
const timeAttribute = $("__time");
expect(cube.dimensions.byName.time).to.be.deep.equal(timeDimension(timeAttribute));
expect(cube.dimensions.tree).to.be.deep.equal(["time"]);
});

it("should override invalid time Attribute", () => {
const cube = fromConfig({ ...baseCube, timeAttribute: "foobar" }, druidCluster);
const timeAttribute = $("__time");
expect(cube.timeAttribute).to.be.equivalent(timeAttribute);
});
});

describe("Native clusters", () => {
const baseCube: DataCubeJS = {
name: "medals",
clusterName: "native",
source: "medals.json"
};

const timeDimensionJS: DimensionJS = {
name: "time",
kind: "time",
formula: "$time_column"
};

it("should throw without timeAttribute property", () => {
expect(() => fromConfig({ ...baseCube })).to.throw("DataCube \"medals\" must have defined timeAttribute property");
});

it("should throw with timeAttribute property pointing to non-existing dimension", () => {
expect(() => fromConfig({ ...baseCube, timeAttribute: "foobar", dimensions: [] })).to.throw("In DataCube \"medals\" could not find dimension for supplied timeAttribute \"foobar\"");
});

it("should pass well defined dimensions and timeAttribute", () => {
const cube = fromConfig({ ...baseCube, timeAttribute: "time_column", dimensions: [timeDimensionJS] });
const timeAttribute = $("time_column");
expect(cube.timeAttribute).to.be.equivalent(timeAttribute);
expect(cube.dimensions).to.be.deep.equal(dimensionsFromConfig([timeDimensionJS]));
});
});
});
});
84 changes: 61 additions & 23 deletions src/common/models/data-cube/data-cube.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ import {
External,
RefExpression
} from "plywood";
import { quoteNames, verifyUrlSafeName } from "../../utils/general/general";
import { isTruthy, quoteNames, verifyUrlSafeName } from "../../utils/general/general";
import { Cluster } from "../cluster/cluster";
import { Dimension, DimensionKind, timeDimension } from "../dimension/dimension";
import { Dimension, DimensionKind, timeDimension as createTimeDimension } from "../dimension/dimension";
import {
allDimensions,
ClientDimensions,
Expand Down Expand Up @@ -117,7 +117,7 @@ export interface DataCube {

dimensions: Dimensions;
measures: Measures;
timeAttribute?: RefExpression;
timeAttribute: RefExpression;
defaultTimezone: Timezone;
defaultFilter?: Filter;
defaultSplitDimensions: string[];
Expand Down Expand Up @@ -178,7 +178,7 @@ export interface SerializedDataCube {

dimensions: SerializedDimensions;
measures: SerializedMeasures;
timeAttribute?: string;
timeAttribute: string;
defaultTimezone: string;
defaultFilter?: FilterJS;
defaultSplitDimensions?: string[];
Expand All @@ -204,7 +204,7 @@ export interface ClientDataCube {

dimensions: ClientDimensions;
measures: Measures;
timeAttribute?: string;
timeAttribute: string;
defaultTimezone: Timezone;
defaultFilter?: Filter;
defaultSplitDimensions?: string[];
Expand Down Expand Up @@ -256,8 +256,12 @@ function readName(config: DataCubeJS): string {
return name;
}

// TODO: this function should return Sum type DruidCluster<Druid> | NativeCluster<void> and we should use it in all DataCube types below
function verifyCluster(config: DataCubeJS, cluster?: Cluster) {
if (cluster === undefined) return;
if (config.clusterName === "native") return;
if (cluster === undefined) {
throw new Error(`Could not find non-native cluster with name "${config.clusterName}" for data cube "${config.name}"`);
}
if (config.clusterName !== cluster.name) {
throw new Error(`Cluster name '${config.clusterName}' was given but '${cluster.name}' cluster was supplied (must match)`);
}
Expand All @@ -274,27 +278,48 @@ function readAttributes(config: DataCubeJS): Pick<DataCube, "attributes" | "attr
};
}

function readTimeAttribute(config: DataCubeJS, cluster?: Cluster): RefExpression | undefined {
const timeAttributeName = config.timeAttribute;
if (timeAttributeName) return $(timeAttributeName);
if (cluster && cluster.type === "druid" && !timeAttributeName) {
return $("__time");
function readTimeAttribute(config: DataCubeJS, cluster: Cluster | undefined, dimensions: Dimensions): { dimensions: Dimensions, timeAttribute: RefExpression } {
const isFromDruidCluster = config.clusterName !== "native" && cluster.type === "druid";
if (isFromDruidCluster) {
if (!isTruthy(config.timeAttribute)) {
console.warn(`DataCube "${config.name}" should have property timeAttribute. Setting timeAttribute to default value "__time"`);
}
if (isTruthy(config.timeAttribute) && config.timeAttribute !== "__time") {
console.warn(`timeAttribute in DataCube "${config.name}" should have value "__time" because it is required by Druid. Overriding timeAttribute to "__time"`);
}
const timeAttribute = $("__time");
if (!isTruthy(findDimensionByExpression(dimensions, timeAttribute))) {
return {
timeAttribute,
dimensions: prepend(createTimeDimension(timeAttribute), dimensions)
};
} else {
return { timeAttribute, dimensions };
}
} else {
if (!isTruthy(config.timeAttribute)) {
throw new Error(`DataCube "${config.name}" must have defined timeAttribute property`);
}
const timeAttribute = $(config.timeAttribute);
const timeDimension = findDimensionByExpression(dimensions, timeAttribute);
if (!isTruthy(timeDimension)) {
throw new Error(`In DataCube "${config.name}" could not find dimension for supplied timeAttribute "${config.timeAttribute}" (must match)`);
}
return {
timeAttribute,
dimensions
};
}
return undefined;
}

function readDimensions(config: DataCubeJS, timeAttribute?: RefExpression): Dimensions {
const dimensions = dimensionsFromConfig(config.dimensions || []);
if (timeAttribute && findDimensionByExpression(dimensions, timeAttribute) === null) {
return prepend(timeDimension(timeAttribute), dimensions);
}
return dimensions;
function readDimensions(config: DataCubeJS): Dimensions {
return dimensionsFromConfig(config.dimensions || []);
}

function readColumns(config: DataCubeJS, timeAttribute: RefExpression): { dimensions: Dimensions, measures: Measures } {
function readColumns(config: DataCubeJS): { dimensions: Dimensions, measures: Measures } {
const name = config.name;
try {
const dimensions = readDimensions(config, timeAttribute);
const dimensions = readDimensions(config);
const measures = measuresFromConfig(config.measures || []);

checkDimensionsAndMeasuresNamesUniqueness(dimensions, measures, name);
Expand Down Expand Up @@ -331,8 +356,8 @@ export function fromConfig(config: DataCubeJS & LegacyDataCubeJS, cluster?: Clus
const { attributes, attributeOverrides, derivedAttributes } = readAttributes(config);

const refreshRule = config.refreshRule ? RefreshRule.fromJS(config.refreshRule) : RefreshRule.query();
const timeAttribute = readTimeAttribute(config, cluster);
const { dimensions, measures } = readColumns(config, timeAttribute);
const { dimensions: initialDimensions, measures } = readColumns(config);
const { timeAttribute, dimensions } = readTimeAttribute(config, cluster, initialDimensions);
verifyDefaultSortMeasure(config, measures);
const subsetFormula = config.subsetFormula || config.subsetFilter;
const defaultFilter = readDefaultFilter(config);
Expand Down Expand Up @@ -458,13 +483,26 @@ export function isTimeAttribute(dataCube: ClientDataCube, ex: Expression) {
return ex instanceof RefExpression && ex.name === dataCube.timeAttribute;
}

export function getTimeDimension(dataCube: ClientDataCube): Dimension {
const dimension = findDimensionByExpression(dataCube.dimensions, $(dataCube.timeAttribute));
if (dimension === null) {
throw new Error(`Expected DataCube "${dataCube.name}" to have timeAttribute property defined with expression of existing dimension`);
}
return dimension;
}

export function getTimeDimensionReference(dataCube: ClientDataCube): string {
return getTimeDimension(dataCube).name;
}

export function getDefaultFilter(dataCube: ClientDataCube): Filter {
const filter = dataCube.defaultFilter || DEFAULT_DEFAULT_FILTER;
if (!dataCube.timeAttribute) return filter;
const timeDimensionReference = getTimeDimensionReference(dataCube);
return filter.insertByIndex(0, new RelativeTimeFilterClause({
period: TimeFilterPeriod.LATEST,
duration: dataCube.defaultDuration,
reference: dataCube.timeAttribute
reference: timeDimensionReference
}));
}

Expand Down
Loading

0 comments on commit b809af8

Please sign in to comment.