feat: convert dataset to table class #1239

heshan0131 · 2020-08-18T00:06:49Z

add valueAccessor to Kepler field, remove tableFieldIdx
Move filter dataset in to table class
Move field domain calculation into table class

heshan0131 · 2021-01-28T15:22:55Z

test/node/utils/index.js

-import './color-util-test';
-import './util-test';
-import './export-utils-test';
+// import './gpu-filter-utils-test';


all these tests are disabled?

heshan0131 · 2021-01-29T02:03:20Z

src/components/map-container.js

@@ -503,7 +504,7 @@ export default function MapContainerFactory(MapPopover, MapControl, Editor) {
        transformRequest
      };

-      const isEdit = mapControls.mapDraw.active;
+      const isEdit = !isExport && mapControls.mapDraw.active;


this now has confict with master

Signed-off-by: Ib Green <[email protected]>

Signed-off-by: Shan He <[email protected]>

Signed-off-by: Giuseppe Macri <[email protected]>

ibgreen · 2021-01-29T14:40:23Z

@igorDykhta Sounds like this is coming together, you can probably rebase your work on this now.

igorDykhta

Looks good in general.
Not sure about inconsistent fieldIdx in several tests and some comments leftovers.

igorDykhta · 2021-02-01T12:49:45Z

src/layers/base-layer.js

@@ -900,8 +895,8 @@ export default class Layer {
   * @returns {object} layer
   */
  updateLayerDomain(datasets, newFilter) {
-    const dataset = this.getDataset(datasets);
-    if (!dataset) {
+    const table = this.getDataset(datasets);


Interchangeable table/dataset variables are quite confusing. Do we plan to unify this in the future?

yea i agree, let's just keep calling it dataset in kepler.gl, because it is used everywhere

igorDykhta · 2021-02-01T13:06:35Z

src/processors/data-processor.js

@@ -140,6 +140,7 @@ export function processCsvData(rawData, header) {
  // here we get a list of none null values to run analyze on
  const sample = getSampleForTypeAnalyze({fields: headerRow, allData: rows});
  const fields = getFieldsFromData(sample, headerRow);
+  // console.log(JSON.stringify(fields, null, 2));


Comment left after testing.

igorDykhta · 2021-02-01T17:41:12Z

test/fixtures/state-saved-v0.js

-    analyzerType: 'INT'
+    fieldIdx: 9,
+    analyzerType: 'INT',
+    valueAccessor: values => values[0]


Not exactly sure about underlying test logic, but should it be values[9]?

the test logic only validates the presence of the valueAccessor. it's part of test field. It's really difficult to compare functions and that's why we only validate the presence and the validity of the field, e.g not null.
I can definitely update to valueAccessor: values => values[9] but it will not make a difference.

igorDykhta · 2021-02-01T17:45:20Z

test/fixtures/test-csv-data.js

-    analyzerType: 'FLOAT'
+    fieldIdx: 5,
+    analyzerType: 'FLOAT',
+    valueAccessor: values => values[4]


values[5] ?

same as above

igorDykhta · 2021-02-01T17:59:04Z

test/node/reducers/vis-state-merger-test.js

+  //   mergedEpochFilter
+  // ];
+  //
+  // cmpFilters(t, expectedFilters, mergedState.filters);


Do we need this commented out check?

yea, we should test these merged filters

igorDykhta · 2021-02-01T18:00:03Z

test/node/reducers/vis-state-test.js

    analyzerType: 'FLOAT',
-    format: ''
+    format: '',
+    valueAccessor: values => values[1]


same as the above

heshan0131 · 2021-02-02T10:09:46Z

src/layers/index.d.ts

@@ -79,17 +74,16 @@ export class Layer {
  _oldDataUpdateTriggers: any;
  hasAllColumns(): boolean;
  updateLayerConfig(p: Partial<LayerConfig>): Layer;
-  updateLayerDomain(datasets: Datasets, filter?: Filter): Layer;


why is this type def deleted? base-layer still has updateLayerDomain method

not sure why this was deleted but i will revert it back

probably some rebase conflicts
same for

isLayerHovered(objectInfo: any): boolean; hasHoveredObject(objectInfo: any): any | null; getHoverData(object: any, allData?: Dataset['allData'], fields?: Dataset['fields']): any;

heshan0131 · 2021-02-02T10:30:28Z

src/reducers/vis-state-updaters.js

@@ -481,12 +482,12 @@ export function layerVisualChannelChangeUpdater(state, action) {
  if (!oldLayer.config.dataId) {
    return state;
  }
-  const dataset = state.datasets[oldLayer.config.dataId];
+  const table = state.datasets[oldLayer.config.dataId];


change this back to dataset?

heshan0131 · 2021-02-02T10:30:34Z

src/reducers/vis-state-updaters.js


  const idx = state.layers.findIndex(l => l.id === oldLayer.id);
  const newLayer = oldLayer.updateLayerConfig(newConfig);

-  newLayer.updateLayerVisualChannel(dataset, channel);
+  newLayer.updateLayerVisualChannel(table, channel);


heshan0131 · 2021-02-02T10:37:45Z

src/utils/filter-utils.js

@@ -962,45 +909,42 @@ export function applyFiltersToDatasets(datasetIds, datasets, filters, layers) {
 export function applyFilterFieldName(filter, dataset, fieldName, filterDatasetIndex = 0, option) {
  // using filterDatasetIndex we can filter only the specified dataset
  const mergeDomain = option && option.hasOwnProperty('mergeDomain') ? option.mergeDomain : false;
-  const {fields, allData} = dataset;
+  // const {fields, allData} = dataset;


delete comment

heshan0131 · 2021-02-02T10:38:46Z

src/utils/filter-utils.js

-    field.hasOwnProperty('filterProps') && field.filterProps
-      ? field.filterProps
-      : getFilterProps(allData, field);
+  // const field = fields[fieldIndex];


delete comment

heshan0131 · 2021-02-02T10:38:54Z

src/utils/filter-utils.js

-  };
-
-  const newFields = Object.assign([...fields], {[fieldIndex]: fieldWithFilterProps});
+  // const fieldWithFilterProps = {


delete comment

heshan0131 · 2021-02-02T11:20:37Z

test/fixtures/test-csv-data.js

-    tableFieldIndex: 10,
-    analyzerType: 'DATETIME'
+    analyzerType: 'DATETIME',
+    valueAccessor: values => values[9]


this doesn't seem to be right, ts valueAccessor should return epoch milisecond, are these really tested?

heshan0131 · 2021-02-02T11:21:07Z

test/fixtures/test-csv-data.js

-    tableFieldIndex: 9,
-    analyzerType: 'DATETIME'
+    analyzerType: 'DATETIME',
+    valueAccessor: values => values[8]


Same as above

heshan0131 · 2021-02-02T11:22:04Z

test/helpers/comparison-utils.js

+          });
+        }
+      } else if (k === 'valueAccessor') {
+        t.ok(typeof actual[k] === 'function', `${name}.valueAccessor should be a function`);


we should actually test valueAccessor not that it's just a function

got it. it's not easy to test equality (not memory address but value) among functions, how should we test it?
I was thinking about the following options:

calling the function with some data and check that both function return the same value? this may have other implications, e.g. sometimes we only compare layers and we don't have the actual data.

use toString on both functions and compare the output?

ok i guess it's fine. the result of valueAccessor is indirectly tested in format layer data and filter.

heshan0131 · 2021-02-02T11:23:27Z

test/node/reducers/vis-state-merger-test.js

+  //   mergedEpochFilter
+  // ];
+  //
+  // cmpFilters(t, expectedFilters, mergedState.filters);


yea, we should test these merged filters

heshan0131 · 2021-02-02T16:58:26Z

test/node/reducers/vis-state-merger-test.js

@@ -400,39 +400,6 @@ test('VisStateMerger.v1 -> mergeLayers -> toEmptyState', t => {
  t.end();
 });

-test('VisStateMerger.v1 -> mergeLayers -> toEmptyState', t => {


this test deleted?

oh yeah, this test was a duplicate of the one at line 370

heshan0131 · 2021-02-02T17:00:46Z

src/utils/filter-utils.js

-  const isTime = field.type === ALL_FIELD_TYPES.timestamp;
-  const valueAccessor = maybeToDate.bind(null, isTime, fieldIdx, field.format);
-  let domain;
+// export function getFieldDomain(allData, field) {


remove comment

Signed-off-by: Giuseppe Macri <[email protected]>

heshan0131 requested review from macrigiuseppe and ibgreen August 18, 2020 00:06

macrigiuseppe force-pushed the feature/field-value-accessor branch from 5b20ed2 to ec2fc7a Compare November 3, 2020 03:59

macrigiuseppe force-pushed the feature/field-value-accessor branch from ec2fc7a to 1507458 Compare January 25, 2021 00:17

macrigiuseppe changed the title ~~[WIP] convert dataset to table class~~ feat: convert dataset to table class Jan 25, 2021

macrigiuseppe force-pushed the feature/field-value-accessor branch from a6c098c to 8627384 Compare January 25, 2021 10:04

heshan0131 requested review from ilyabo and mletic January 25, 2021 12:12

igorDykhta self-requested a review January 28, 2021 15:10

heshan0131 commented Jan 28, 2021

View reviewed changes

macrigiuseppe force-pushed the feature/field-value-accessor branch 2 times, most recently from bbfcd58 to 1cc8c16 Compare January 28, 2021 16:43

heshan0131 commented Jan 29, 2021

View reviewed changes

ibgreen and others added 13 commits January 29, 2021 14:12

Introduce KeplerTable class (#1112)

56384c1

Signed-off-by: Ib Green <[email protected]>

Add value accessor to field

f43abd1

Signed-off-by: Shan He <[email protected]>

wip

f36b1f8

Signed-off-by: Shan He <[email protected]>

move filter and layer calculation to table class

b653beb

Signed-off-by: Shan He <[email protected]>

beginning fixing tests

49f75c8

Signed-off-by: Shan He <[email protected]>

Fixed data processor test

5935578

Signed-off-by: Giuseppe Macri <[email protected]>

Fixed data-processor-test

c170629

Signed-off-by: Giuseppe Macri <[email protected]>

Fixed node tests

8e93715

Signed-off-by: Giuseppe Macri <[email protected]>

Fixed browser tests

6db5576

Signed-off-by: Giuseppe Macri <[email protected]>

fixed isEdit when isExport is true

81b1ac3

Signed-off-by: Giuseppe Macri <[email protected]>

Fixed kepler table changed filters prop

f2586c0

Signed-off-by: Giuseppe Macri <[email protected]>

Fixed tests

87c59ba

Signed-off-by: Giuseppe Macri <[email protected]>

Fixed conflicts

7993342

Signed-off-by: Giuseppe Macri <[email protected]>

macrigiuseppe force-pushed the feature/field-value-accessor branch from 1cc8c16 to 7993342 Compare January 29, 2021 13:21

igorDykhta reviewed Feb 1, 2021

View reviewed changes

heshan0131 commented Feb 2, 2021

View reviewed changes

Removed comments and update test cases

d20eff8

Signed-off-by: Giuseppe Macri <[email protected]>

macrigiuseppe force-pushed the feature/field-value-accessor branch from a81f141 to d20eff8 Compare February 2, 2021 17:07

macrigiuseppe merged commit 9476c29 into master Feb 2, 2021

delete-merged-branch bot deleted the feature/field-value-accessor branch February 2, 2021 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: convert dataset to table class #1239

feat: convert dataset to table class #1239

heshan0131 commented Aug 18, 2020

heshan0131 Jan 28, 2021

heshan0131 Jan 29, 2021

macrigiuseppe Jan 29, 2021

ibgreen commented Jan 29, 2021

igorDykhta left a comment

igorDykhta Feb 1, 2021

heshan0131 Feb 2, 2021

igorDykhta Feb 1, 2021

igorDykhta Feb 1, 2021

macrigiuseppe Feb 2, 2021

igorDykhta Feb 1, 2021

macrigiuseppe Feb 2, 2021

igorDykhta Feb 1, 2021

heshan0131 Feb 2, 2021

igorDykhta Feb 1, 2021

macrigiuseppe Feb 2, 2021

heshan0131 Feb 2, 2021

macrigiuseppe Feb 2, 2021 •

edited

Loading

heshan0131 Feb 2, 2021

heshan0131 Feb 2, 2021

heshan0131 Feb 2, 2021

heshan0131 Feb 2, 2021

heshan0131 Feb 2, 2021

heshan0131 Feb 2, 2021

heshan0131 Feb 2, 2021 •

edited

Loading

heshan0131 Feb 2, 2021 •

edited

Loading

heshan0131 Feb 2, 2021

macrigiuseppe Feb 2, 2021 •

edited

Loading

heshan0131 Feb 2, 2021

macrigiuseppe Feb 2, 2021

heshan0131 Feb 2, 2021

heshan0131 Feb 2, 2021

macrigiuseppe Feb 2, 2021

heshan0131 Feb 2, 2021

feat: convert dataset to table class #1239

feat: convert dataset to table class #1239

Conversation

heshan0131 commented Aug 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibgreen commented Jan 29, 2021

igorDykhta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macrigiuseppe Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heshan0131 Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

heshan0131 Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macrigiuseppe Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macrigiuseppe Feb 2, 2021 •

edited

Loading

heshan0131 Feb 2, 2021 •

edited

Loading

heshan0131 Feb 2, 2021 •

edited

Loading

macrigiuseppe Feb 2, 2021 •

edited

Loading