-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
sparse_vector field support #168186
sparse_vector field support #168186
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one question
<EuiToken | ||
aria-label="sparse_vector" | ||
className="kbnFieldIcon" | ||
iconType="tokenDenseVector" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use tokenDenseVector
or should we create a new token for sparse vector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already asked for a specific field icon in elastic/eui#7257. I think we can update that when / if ready.
@@ -69,6 +70,7 @@ export const typeToEuiIconMap: Partial<Record<string, EuiTokenProps>> = { | |||
_source: { iconType: 'editorCodeBlock', color: 'gray' }, | |||
point: { iconType: 'tokenShape' }, // there is no separate icon for `point` yet | |||
shape: { iconType: 'tokenShape' }, | |||
sparse_vector: { iconType: 'tokenDenseVector' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here, do we want to use tokenDenseVector or should we create a new token for sparse vector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreadelrio do we have an icon we can use or add to eui?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Andrea is already pinged in elastic/eui#7257 (comment). 😉 My plan is to open a separate PR when that's ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data Discovery changes LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -851,6 +851,23 @@ export const TYPE_DEFINITION: { [key in DataType]: DataTypeDefinition } = { | |||
</p> | |||
), | |||
}, | |||
sparse_vector: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @carlosdelest, just a question: why are we adding these changes to the Index management plugin?
Did you mean to add support for the sparse_vector
field type in the Mappings editor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying! Tested locally and LGTM.
…_field_support' into carlosdelest/sparse_vector_field_support
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Index management changes LGTM
@@ -851,6 +851,23 @@ export const TYPE_DEFINITION: { [key in DataType]: DataTypeDefinition } = { | |||
</p> | |||
), | |||
}, | |||
sparse_vector: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying! Tested locally and LGTM.
Summary
Add
sparse_vector
field support (added to Elasticsearch here) for Discovery. Specific field icon is pending this EUI issue:Field type:
Field filter:
Field in doc table:
It also uses
sparse_vector
as the field mapping for ELSER based ML pipelines created using Kibana:Checklist
Delete any items that are not applicable to this PR.