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.

Sign up for GitHub

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

Jump to bottom

[Feat] allow for multiple value column type #1666

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

[Feat] allow for multiple value column type #1666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a1cefa9
[Feat] allow for multiple value column type
crabatel Dec 9, 2021
0ee098f
[Fix] Remove react and react-dom from devDependencies to allow npm link
crabatel Dec 23, 2021
bfb12f1
Add dist build for tests
crabatel Dec 15, 2021
944d37d
Update npmpublish.yml
Pipazoul Jan 25, 2022
9a2e929
Update npmpublish.yml
Pipazoul Jan 25, 2022
994ed49
Update package.json
Pipazoul Jan 25, 2022
0c3dcb4
FIX changes icon.json url
Pipazoul Jan 25, 2022
fa9ffd3
Update package.json
Pipazoul Jan 25, 2022
f51f6e7
Update package.json
Pipazoul Jan 25, 2022
49bdadd
FIX icon url
Pipazoul Jan 25, 2022
b800a54
Merge branch 'add-multiple-value-column' of https://github.com/datatl…
Pipazoul Jan 25, 2022
5e45e73
Delete dist directory
Pipazoul Jan 26, 2022
9f4ff24
revert npmpublish.yml file
Pipazoul Jan 27, 2022
7f382d6
revert package.json
Pipazoul Jan 27, 2022
32bb467
revert default-setting.js
Pipazoul Jan 27, 2022
de75a33
revert icon-layer.js
Pipazoul Jan 27, 2022
842b667
revert constants.js
Pipazoul Jan 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/npmpublish.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name: Node.js Package
on:
release:
types: [created]
workflow_dispatch:

jobs:
publish-npm:
Expand Down
2 changes: 0 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,6 @@
"prettier": "1.19.1",
"progress-bar-webpack-plugin": "^2.1.0",
"puppeteer": "^2.1.1",
"react": "^16.8.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why this is removed?

Copy link
Author

Choose a reason for hiding this comment

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

Hello,
Actually I removed this because I thought having react as a dependency there was the cause of an error when trying to npm link the Kepler package (@see https://stackoverflow.com/questions/66975613/error-invalid-hook-call-hooks-can-only-be-called-inside-of-the-body-of-a-funct), but it appeared it wasn't the reason as we could not get rid of this error.

As we could not npm link we resolved to pushing the dist/ folder on our repo and updating our dependency to point to our repo, hence the presence of the dist/ folder.

Concerning this PR, we were not sure it could be accepted as we made the assumption that an array typed column in the CSV would be in a JSON format. This is the format we decided to use for our project, but it may not be the case for everyone, and maybe it should be discussed.

If the changes can be accepted, I'll remove the dist/ from this branch as well as the dependency change.

"react-dom": "^16.8.4",
"react-hot-loader": "^4.13.0",
"redux-mock-store": "^1.2.1",
"sinon": "^2.4.1",
Expand Down
6 changes: 6 additions & 0 deletions src/constants/default-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ export const ALL_FIELD_TYPES = keyMirror({
real: null,
string: null,
timestamp: null,
array: null,
point: null
});

Expand Down Expand Up @@ -382,6 +383,7 @@ const BLUE = '140, 210, 205';
const BLUE2 = '106, 160, 206';
const BLUE3 = '0, 172, 237';
const GREEN = '106, 160, 56';
const GREEN2 = '23, 207, 114';
const RED = '237, 88, 106';

export const FILED_TYPE_DISPLAY = {
Expand Down Expand Up @@ -413,6 +415,10 @@ export const FILED_TYPE_DISPLAY = {
label: 'time',
color: GREEN
},
[ALL_FIELD_TYPES.array]: {
label: 'array',
color: GREEN2
},
// field pairs
[ALL_FIELD_TYPES.point]: {
label: 'point',
Expand Down
10 changes: 8 additions & 2 deletions src/processors/data-processor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ export const PARSE_FIELD_VALUE_FROM_STRING = {
['x', 'X'].includes(field.format) ? typeof d === 'number' : typeof d === 'string',
parse: (d, field) => (['x', 'X'].includes(field.format) ? Number(d) : d)
},
[ALL_FIELD_TYPES.array]: {
valid: d => Array.isArray(d),
parse: d => JSON.parse(d)
},
[ALL_FIELD_TYPES.real]: {
valid: d => parseFloat(d) === d,
// Note this will result in NaN for some string
Expand Down Expand Up @@ -292,6 +296,7 @@ export function getFieldsFromData(data, fieldOrder) {
const metadata = Analyzer.computeColMeta(
data,
[
{regex: /.*array/g, dataType: 'ARRAY'},
{regex: /.*geojson|all_points/g, dataType: 'GEOMETRY'},
{regex: /.*census/g, dataType: 'STRING'}
],
Expand Down Expand Up @@ -393,12 +398,13 @@ export function analyzerTypeToFieldType(aType) {
return ALL_FIELD_TYPES.integer;
case BOOLEAN:
return ALL_FIELD_TYPES.boolean;
case ARRAY:
return ALL_FIELD_TYPES.array;
case GEOMETRY:
case GEOMETRY_FROM_STRING:
case PAIR_GEOMETRY_FROM_STRING:
case ARRAY:
case OBJECT:
// TODO: create a new data type for objects and arrays
// TODO: create a new data type for objects
return ALL_FIELD_TYPES.geojson;
case NUMBER:
case STRING:
Expand Down
2 changes: 1 addition & 1 deletion src/utils/data-scale-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export function getQuantileDomain(data, valueAccessor, sortFunc) {
export function getOrdinalDomain(data, valueAccessor) {
const values = typeof valueAccessor === 'function' ? data.map(valueAccessor) : data;

return unique(values)
return unique(values.flat()) // Flatten in case values are of type array
.filter(notNullorUndefined)
.sort();
}
Expand Down
2 changes: 2 additions & 0 deletions src/utils/data-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,8 @@ export const FIELD_DISPLAY_FORMAT = {
[ALL_FIELD_TYPES.real]: defaultFormatter,
[ALL_FIELD_TYPES.boolean]: defaultFormatter,
[ALL_FIELD_TYPES.date]: defaultFormatter,
[ALL_FIELD_TYPES.array]: d =>
typeof d === 'string' ? d : Array.isArray(d) ? `[${String(d)}]` : '',
[ALL_FIELD_TYPES.geojson]: d =>
typeof d === 'string'
? d
Expand Down
10 changes: 9 additions & 1 deletion src/utils/filter-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ export function getFilterProps(field, fieldDomain) {
gpu: false
};

case ALL_FIELD_TYPES.array:
case ALL_FIELD_TYPES.string:
case ALL_FIELD_TYPES.date:
return {
Expand Down Expand Up @@ -413,7 +414,13 @@ export function getFilterFunction(field, dataId, filter, layers) {
case FILTER_TYPES.range:
return data => isInRange(valueAccessor(data), filter.value);
case FILTER_TYPES.multiSelect:
return data => filter.value.includes(valueAccessor(data));
return data => {
const value = valueAccessor(data);
if (Array.isArray(value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To make this function more efficient, we should do the checking out side the data => boolean function

case FILTER_TYPES.multiSelect:
return feld.type === ALL_FILED_TYPES.array ? data => {
  const value = valueAccessor(data);
  return Array.isArray(value) && value.some(v => filter.value.includes(v));
} : data => filter.value.includes(valueAccessor(data));

Copy link
Author

Choose a reason for hiding this comment

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

Indeed it's better, I'll update this one.

return value.some(v => filter.value.includes(v));
}
return filter.value.includes(value);
};
case FILTER_TYPES.select:
return data => valueAccessor(data) === filter.value;
case FILTER_TYPES.timeRange:
Expand Down Expand Up @@ -941,6 +948,7 @@ export function mergeFilterDomainStep(filter, filterProps) {
};

switch (filterProps.fieldType) {
case ALL_FIELD_TYPES.array:
case ALL_FIELD_TYPES.string:
case ALL_FIELD_TYPES.date:
return {
Expand Down
1 change: 1 addition & 0 deletions src/utils/table-utils/kepler-table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ class KeplerTable {
case ALL_FIELD_TYPES.boolean:
return {domain: [true, false]};

case ALL_FIELD_TYPES.array:
case ALL_FIELD_TYPES.string:
case ALL_FIELD_TYPES.date:
domain = getOrdinalDomain(allData, valueAccessor);
Expand Down
4 changes: 2 additions & 2 deletions test/fixtures/geojson.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ export const geoStyleFields = [
displayName: 'fillColor',
format: '',
fieldIdx: 1,
type: 'geojson',
type: 'array',
analyzerType: 'ARRAY',
valueAccessor: values => values[1]
},
Expand All @@ -567,7 +567,7 @@ export const geoStyleFields = [
displayName: 'lineColor',
format: '',
fieldIdx: 2,
type: 'geojson',
type: 'array',
analyzerType: 'ARRAY',
valueAccessor: values => values[2]
},
Expand Down
21 changes: 21 additions & 0 deletions test/node/utils/data-processor-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,27 @@ test('Processor -> parseCsvRowsByFieldType -> boolean', t => {
t.end();
});

test('Processor -> parseCsvRowsByFieldType -> array', t => {
const field = {
type: ALL_FIELD_TYPES.array
};

const rows = [
['[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]'],
['["tag1", "tag2", "tag3", "tag4", "tag5", "tag6", "tag7"]']
];

const expected = [
[[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]],
[['tag1', 'tag2', 'tag3', 'tag4', 'tag5', 'tag6', 'tag7']]
];

parseCsvRowsByFieldType(rows, -1, field, 0);

t.same(rows, expected, 'should parsed arrays properly');
t.end();
});

test('Processor -> getSampleForTypeAnalyze', t => {
const fields = ['string', 'int', 'bool', 'time'];

Expand Down