-
Notifications
You must be signed in to change notification settings - Fork 1
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
Attribute with repeated data type. #991
Changes from 19 commits
32a2656
eb79192
8fe09c8
5d54178
d4b6921
519f3cd
c209b0f
0620795
0714da8
dd3f527
4e8c32c
3e4bce0
1ae4e10
8eb84dc
66b6633
ea8dff6
3a5b7e9
7f8f84d
2f25ffe
89bfadb
906b06d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1432,6 +1432,12 @@ components: | |
type: string | ||
description: Optional display string | ||
nullable: true | ||
repeatedValue: | ||
type: array | ||
items: | ||
$ref: "#/components/schemas/Literal" | ||
isRepeatedValue: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this for? Doesn't the existence of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to distinguish an empty array from a non-repeated/scalar value. Some of the rows in the variant table have empty arrays. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will there potentially be other types as well and this should be an enum? Should There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have plans for any other types, though if they come up then an enum certainly sounds like a good choice to consider. I'm happy to remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per offline discussion, we decided to:
|
||
type: boolean | ||
|
||
Literal: | ||
type: object | ||
|
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.
Is the existence of a display field not independent of whether the value is an array?
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.
Currently, it's impossible to have both a display string and a repeated value. In the future, we could support that though, or maybe it would be better to support an array of value-display pairs, not sure. For the
variant
entity, we only have arrays of strings.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 guess that's what I'm wondering and trying to see if we're baking in the way variant search uses arrays the right amount. The array of values as used here seems like it would map to a single display string. Presumably this works differently when a DisplayValue object is returned as part of an enum hint and there wouldn't be repeated values there, but maybe that's not always true? Should some places have a list of ValueDisplays instead of a single one? Should the array be inside Literal instead? Maybe there are two versions, this isn't a general array but specifically support for a list of repeated, individual items, and there could exist another where the array is treated as a single unit?
I don't have a particular proposal and I'm just trying to understand the model and how it will work going forward.
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.
Currently the backend (indexing & query engine) does not support repeated values that have both a value & display component. I think that would be a good enhancement, but it's not needed for variant search and we don't have any other immediate use cases, so in the interest of time I decided to tackle this smaller change first.
I don't think array should be inside literal, because then we'd need a new data type for each array (e.g. string-array, int64-array).
For hints, we don't return repeated values. The indexing calculates counts considering each array element separately. e.g. If a condition could be included in >1 vocabulary (condition.vocabulary is a repeated attribute), then we count the condition once per vocabulary. The returned hints are still vocabulary-count pairs, not array of vocabularies-count pairs.
Currently, the only time the API could return a list of values is when fetching an attribute in a list query. I think we may want to expand this in the future, though probably not all at once here.
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.
Per offline discussion, we decided that in the future it might be useful to support: