[indexPatterns/field] remove indexed/analyzed/doc_values awareness #11969
Conversation
spalger
force-pushed
the
index-patterns/read-from-doc-values
branch
from
de99c16
to
334ebfc
Compare
spalger
force-pushed
the
index-patterns/read-from-doc-values
branch
from
6c8a36d
to
23a30a6
Compare
| 'missing', | ||
| 'name' | ||
| ], | ||
| defaults: { | ||
| missing: true | ||
| missing: true, | ||
| type: 'any' |
There was a problem hiding this comment.
What's the benefit of any over null or undefined?
There was a problem hiding this comment.
That's the value rendered in the UI. This is master:
| @@ -45,7 +44,8 @@ export function AggTypesMetricsTopHitProvider(Private) { | |||
| } | |||
| }; | |||
| } else { | |||
| if (field.doc_values) { | |||
| // https://git.io/vSPvJ | |||
There was a problem hiding this comment.
Why not just the real github url here?
There was a problem hiding this comment.
Habit, linking to specific lines of specific files in specific commits is over 120 characters in many cases, no reason this can't be there though.
| Object.defineProperties(Field.prototype, { | ||
| indexed: { | ||
| get() { | ||
| throw new Error('field.indexed has been removed.'); |
There was a problem hiding this comment.
Whenever we remove features and want to have messages like this, we should maybe follow a pattern like "X has been removed, use Y instead" or "X has been removed, see github.com/Y" to be more helpful
| } | ||
|
|
||
| return indexPattern.fields.every(field => { | ||
| // added in 5.0 #8421 |
There was a problem hiding this comment.
First time I've seen this kind of comment in Kibana (but that might just be because I haven't looked at much code yet). Is there a specific reason for this here?
There was a problem hiding this comment.
I'm pretty sure there are a couple places like this. The main reason I would but these types of comments in is that these lines are here for backwards compatibility, so if we want to remove/edit them we need to consider when they were added. It's not a ton different than most things, but having a note for why this somewhat strange series of checks exists is nothing but helpful if you ask me.
There was a problem hiding this comment.
++ I agree. Enough to link to the PR, e.g. "Added in #x"? (So we don't have the diff between "added in" and "slotted for"). The PR will have the details about version and why it's added etc.
|
|
||
| if (!hasFieldCaps || !hasDocValuesFlag) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Just return !hasFieldCaps || !hasDocValuesFlag, no point in having the if
…lastic#11969) * [indexPatterns/field] replace mapping awareness with readFromDocValues * move link to more details to proper location * Address kjbekkelund's feedback * let the prs speak for themselves
With #11114 we will no longer be able to depend on access to the field mappings, meaning that the field objects will no longer have
field.indexed,field.analyzed, orfield.doc_valuesproperties. In place of these properties fields will get afield.readFromDocValuesproperty that will indicate which fields should be accessed usingdocvalue_fieldswhen reading the field on documents in elasticsearch.See #11141 for reasoning behind how
readFromDocValuesis calculated.