Skip to content

Add support of BinaryFieldValues for non _source primitive array indexing#387

Open
laminelam wants to merge 1 commit intoopensearch-project:mainfrom
laminelam:main
Open

Add support of BinaryFieldValues for non _source primitive array indexing#387
laminelam wants to merge 1 commit intoopensearch-project:mainfrom
laminelam:main

Conversation

@laminelam
Copy link

This is part of a broader contribution to add support of non _source primitive array indexing

For now, only float arrays are supported, as well as BytesValue.

Two options are offered:

  • packed array.
  • binary LE (little-endian) which is much faster (see benchmarks results in the related issues).

Issues Resolved

opensearch-project/opensearch-api-specification#1035
opensearch-project/OpenSearch#19638

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…xing

Signed-off-by: Lamine Idjeraoui <lidjeraoui@apple.com>
Copy link
Collaborator

@karenyrx karenyrx left a comment

Choose a reason for hiding this comment

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

Thanks @laminelam this PR generally looks good, we are waiting on the feature to add back tooling_skip support before merging your PR (as it was recently reverted in https://github.com/opensearch-project/opensearch-protobufs/pull/387/changes)

Comment on lines +891 to +892
// Map of fully-qualified field path -> typed value.
map<string, BinaryFieldValue> field_values = 4 [(tooling_skip) = true];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Map of fully-qualified field path -> typed value.
map<string, BinaryFieldValue> field_values = 4 [(tooling_skip) = true];
// EXPERIMENTAL field - may have breaking changes
// [optional] Map of fully-qualified field path -> typed value.
map<string, BinaryFieldValue> field_values = 4 [(tooling_skip) = true];

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to give us an escape hatch in case this needs any tuning :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally can you document with which operation this should be used for? (create/update/index/delete) and which fields this is not compatible with? e.g. can both object and field_values provided?


message BinaryFieldValue {
oneof binary_field_value {
BytesValue bytes_value = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add some comments above all the newly added fields to clarify their usage, along with the [optional] / [required] keyword to adhere to the current style in the repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants