Bug/timeattribute

src/client/components/filter-tile/filter-tiles-row.tsx

@@ -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";
@@ -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);

src/common/models/data-cube/data-cube.mocha.ts

@@ -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"
@@ -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?");
     });
 
@@ -81,7 +86,7 @@ describe("DataCube", () => {
               formula: "$main.sum($count)"
             }
           ]
-        });
+        }, druidCluster);
       }).to.throw("Can not find defaultSortMeasure 'gaga' in data cube 'wiki'");
     });
 
@@ -108,7 +113,7 @@ describe("DataCube", () => {
               formula: "$main.sum($count)"
             }
           ]
-        });
+        }, druidCluster);
       }).to.throw("data cube: 'wiki', names: 'articleName' found in both dimensions and measures");
     });
 
@@ -139,7 +144,7 @@ describe("DataCube", () => {
               formula: "$articleName"
             }
           ]
-        });
+        }, druidCluster);
       }).to.throw("data cube: 'wiki', found duplicate measure with name: 'articleName'");
     });
 
@@ -170,7 +175,7 @@ describe("DataCube", () => {
               formula: "$main.sum($count)"
             }
           ]
-        });
+        }, druidCluster);
       }).to.throw("data cube: 'wiki', found duplicate dimension with name: 'articleName'");
     });
 
@@ -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", () => {
@@ -656,7 +661,7 @@ describe("DataCube", () => {
             formula: "${added!!!}"
           }
         ]
-      });
+      }, druidCluster);
 
       const dataCube = addAttributes(dataCubeWithDim, attributes1);
       expect(Object.keys(dataCube.measures.byName)).to.deep.equal(["deleted"]);
@@ -676,7 +681,7 @@ describe("DataCube", () => {
       refreshRule: {
         rule: "realtime"
       }
-    });
+    }, druidCluster);
 
     it("adds new dimensions", () => {
       const columns: any = [
@@ -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]));
+      });
+    });
   });
 });

src/common/models/data-cube/data-cube.ts

@@ -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,
@@ -117,7 +117,7 @@ export interface DataCube {
 
   dimensions: Dimensions;
   measures: Measures;
-  timeAttribute?: RefExpression;
+  timeAttribute: RefExpression;
   defaultTimezone: Timezone;
   defaultFilter?: Filter;
   defaultSplitDimensions: string[];
@@ -178,7 +178,7 @@ export interface SerializedDataCube {
 
   dimensions: SerializedDimensions;
   measures: SerializedMeasures;
-  timeAttribute?: string;
+  timeAttribute: string;
   defaultTimezone: string;
   defaultFilter?: FilterJS;
   defaultSplitDimensions?: string[];
@@ -204,7 +204,7 @@ export interface ClientDataCube {
 
   dimensions: ClientDimensions;
   measures: Measures;
-  timeAttribute?: string;
+  timeAttribute: string;
   defaultTimezone: Timezone;
   defaultFilter?: Filter;
   defaultSplitDimensions?: string[];
@@ -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)`);
   }
@@ -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);
@@ -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);
@@ -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
   }));
 }