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

Add multi field info to the IndexPattern #33681

Merged
merged 9 commits into from
Apr 2, 2019
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@
"scripted": false,
"searchable": true,
"aggregatable": true,
"readFromDocValues": true
"readFromDocValues": true,
"parent": "machine.os",
"subType": "multi"
},
{
"name": "geo.src",
Expand Down
10 changes: 7 additions & 3 deletions src/fixtures/logstash_fields.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ function stubbedLogstashFields() {
return [
// |aggregatable
// | |searchable
// name esType | | |metadata
// name esType | | |metadata | parent | subType
['bytes', 'long', true, true, { count: 10 } ],
['ssl', 'boolean', true, true, { count: 20 } ],
['@timestamp', 'date', true, true, { count: 30 } ],
Expand All @@ -41,7 +41,7 @@ function stubbedLogstashFields() {
['geo.coordinates', 'geo_point', true, true ],
['extension', 'keyword', true, true ],
['machine.os', 'text', true, true ],
['machine.os.raw', 'keyword', true, true ],
['machine.os.raw', 'keyword', true, true, {}, 'machine.os', 'multi' ],
['geo.src', 'keyword', true, true ],
['_id', '_id', true, true ],
['_type', '_type', true, true ],
Expand All @@ -59,7 +59,9 @@ function stubbedLogstashFields() {
esType,
aggregatable,
searchable,
metadata = {}
metadata = {},
parent = undefined,
subType = undefined,
] = row;

const {
Expand All @@ -83,6 +85,8 @@ function stubbedLogstashFields() {
script,
lang,
scripted,
parent,
subType,
};
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,6 @@ export interface FieldDescriptor {
readFromDocValues: boolean;
searchable: boolean;
type: string;
parent?: string;
subType?: string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,34 @@
"index1"
]
}
},
"multi_parent": {
"text": {
"type": "text",
"searchable": true,
"aggregatable": false
}
},
"multi_parent.child": {
"keyword": {
"type": "keyword",
"searchable": true,
"aggregatable": true
}
},
"object_parent": {
"object": {
"type": "object",
"searchable": false,
"aggregatable": false
}
},
"object_parent.child": {
"keyword": {
"type": "keyword",
"searchable": true,
"aggregatable": true
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ import { shouldReadFieldFromDocValues } from './should_read_field_from_doc_value
*/
export function readFieldCapsResponse(fieldCapsResponse) {
const capsByNameThenType = fieldCapsResponse.fields;
return Object.keys(capsByNameThenType).map(fieldName => {
const kibanaFormattedCaps = Object.keys(capsByNameThenType).map(fieldName => {
const capsByType = capsByNameThenType[fieldName];
const types = Object.keys(capsByType);

Expand Down Expand Up @@ -120,7 +120,26 @@ export function readFieldCapsResponse(fieldCapsResponse) {
aggregatable: isAggregatable,
readFromDocValues: shouldReadFieldFromDocValues(isAggregatable, esType),
};
}).filter(field => {
});

// Get all types of sub fields. These could be multi fields or children of nested/object types
const subFields = kibanaFormattedCaps.filter(field => {
return field.name.includes('.');
});

// Discern which sub fields are multi fields. If the parent field is not an object or nested field
// the child must be a multi field.
subFields.forEach(field => {
const parentFieldName = field.name.split('.').slice(0, -1).join('.');
const parentFieldCaps = kibanaFormattedCaps.find(caps => caps.name === parentFieldName);

if (parentFieldCaps && !['object', 'nested'].includes(parentFieldCaps.type)) {
field.parent = parentFieldName;
field.subType = 'multi';
Copy link
Member

Choose a reason for hiding this comment

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

Why not just have a isMulti boolean instead of adding another string property subType? What else could subType be?

Copy link
Member

Choose a reason for hiding this comment

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

Do we even need the multi designation? Can we just assume if parent is not undefined, it's multi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future it could be nested, object, or join. I see this being used to describe any parent/child relationship between fields. I thought about just doing an isMulti flag but then we'd need to add isNested, isObject and isJoin in the future.

Instead of having a separate parent field and a flag we could also just have something like multiParent, nestedParent, objectParent, joinParent. We'd still have to add a new one for each new relationship type, but then they're not coupled together.

I dunno, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made all the updates I can without making a decision on this. Once we've agreed on a format I'll go through and update all the other places mentioned that need these properties.

}
});

return kibanaFormattedCaps.filter(field => {
return !['object', 'nested'].includes(field.type);
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

/* eslint import/no-duplicates: 0 */
import { cloneDeep } from 'lodash';
import { cloneDeep, omit } from 'lodash';
import sinon from 'sinon';

import * as shouldReadFieldFromDocValuesNS from './should_read_field_from_doc_values';
Expand All @@ -37,21 +37,20 @@ describe('index_patterns/field_capabilities/field_caps_response', () => {
describe('conflicts', () => {
it('returns a field for each in response, no filtering', () => {
const fields = readFieldCapsResponse(esResponse);
expect(fields).toHaveLength(19);
expect(fields).toHaveLength(22);
});

it('includes only name, type, searchable, aggregatable, readFromDocValues, and maybe conflictDescriptions of each field', () => {
it('includes only name, type, searchable, aggregatable, readFromDocValues, and maybe conflictDescriptions, parent, ' +
'and subType of each field', () => {
const responseClone = cloneDeep(esResponse);
// try to trick it into including an extra field
responseClone.fields['@timestamp'].date.extraCapability = true;
const fields = readFieldCapsResponse(responseClone);

fields.forEach(field => {
if (field.conflictDescriptions) {
delete field.conflictDescriptions;
}
const fieldWithoutOptionalKeys = omit(field, 'conflictDescriptions', 'parent', 'subType');

expect(Object.keys(field)).toEqual([
expect(Object.keys(fieldWithoutOptionalKeys)).toEqual([
'name',
'type',
'searchable',
Expand All @@ -65,7 +64,8 @@ describe('index_patterns/field_capabilities/field_caps_response', () => {
sandbox.spy(shouldReadFieldFromDocValuesNS, 'shouldReadFieldFromDocValues');
const fields = readFieldCapsResponse(esResponse);
const conflictCount = fields.filter(f => f.type === 'conflict').length;
sinon.assert.callCount(shouldReadFieldFromDocValues, fields.length - conflictCount);
// +1 is for the object field which gets filtered out of the final return value from readFieldCapsResponse
sinon.assert.callCount(shouldReadFieldFromDocValues, fields.length - conflictCount + 1);
});

it('converts es types to kibana types', () => {
Expand Down Expand Up @@ -121,6 +121,23 @@ describe('index_patterns/field_capabilities/field_caps_response', () => {
expect(mixSearchable.searchable).toBe(true);
expect(mixSearchableOther.searchable).toBe(true);
});

it('returns multi fields with parent and subType keys describing the relationship', () => {
const fields = readFieldCapsResponse(esResponse);
const child = fields.find(f => f.name === 'multi_parent.child');
expect(child).toHaveProperty('parent', 'multi_parent');
expect(child).toHaveProperty('subType', 'multi');
});

it('should not confuse object children for multi field children', () => {
// We detect multi fields by finding fields that have a dot in their name and then looking
// to see if their parents are *not* object or nested fields. In the future we may want to
// add parent and subType info for object and nested fields but for now we don't need it.
const fields = readFieldCapsResponse(esResponse);
const child = fields.find(f => f.name === 'object_parent.child');
expect(child).not.toHaveProperty('parent');
expect(child).not.toHaveProperty('subType');
});
});
});
});

Large diffs are not rendered by default.

Loading