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

feature: Pass the value of the expression through parameters #331

Merged

Conversation

xiaocai2333
Copy link
Contributor

@sre-ci-robot sre-ci-robot requested review from czs007 and yhmo October 22, 2024 02:10
@mergify mergify bot added dco-passed DCO check passed. ci-passed labels Oct 22, 2024
@xiaocai2333 xiaocai2333 force-pushed the add_placehiolder_for_expression branch 2 times, most recently from 7137c5d to 6d6b611 Compare October 22, 2024 06:53
@@ -905,6 +905,7 @@ message DeleteRequest {
string expr = 5;
repeated uint32 hash_keys = 6;
common.ConsistencyLevel consistency_level = 7;
repeated schema.FieldData expression_values = 8;
Copy link
Member

Choose a reason for hiding this comment

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

Plz CMIIW, in #37033 we use the following code to read the value out of this FieldData

result[data.GetFieldName()] = rv[0]

And use value, ok := data[expr.GetPlaceholderName()] to fill the expr. And result above equals to data here.

So our assumption is that the placeholder name is the field name?

Also, can you provide some examples showing how we will fill this expression_values?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think here FieldData is only used as a container to hold the template variable name: value mapping.

@@ -905,6 +905,7 @@ message DeleteRequest {
string expr = 5;
repeated uint32 hash_keys = 6;
common.ConsistencyLevel consistency_level = 7;
repeated schema.FieldData expression_values = 8;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder is schema.FieldData the best container to use? schema.PlaceholderValue seems better.

Copy link
Contributor

Choose a reason for hiding this comment

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

when using schema.FieldData we assigned new meaning to FieldData::field_name

@xiaocai2333 xiaocai2333 force-pushed the add_placehiolder_for_expression branch from 6d6b611 to 78665e9 Compare October 23, 2024 08:24
@zhengbuqian
Copy link
Contributor

chatted with @czs007 offline:

  • the main reason we are using schema.FieldData is to utilize the Base 128 variant encoding capability to have some levels of compression on int fields.
    • this encoding doesn't apply to floating types
    • it also doesn't apply to string types
    • Technically Base 128 variant is NOT a compression algorithm. Protobuf does not have built-in compression.
  • using fieldName member of schema.FieldData as the template variable name is indeed confusing. And it will be painful if we want to modify this due to the need of backward compatibility.

maybe the best option is to define a new proto message similar to FieldData but with correct member naming.

@zhengbuqian
Copy link
Contributor

defining a new proto message also reduces complexity in impl. we need only a single value whereas schema.FieldData is a repeated, currently in convert_field_data_to_generic_value.go we consistently loop through the repeated field and then assert its size to be 1. In the new proto we can directly define it as not repeated.

@xiaocai2333 xiaocai2333 force-pushed the add_placehiolder_for_expression branch from 78665e9 to 1d03d83 Compare October 25, 2024 03:00
@yhmo
Copy link
Collaborator

yhmo commented Oct 25, 2024

/lgtm
/approve

@sre-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xiaocai2333, yhmo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot sre-ci-robot merged commit 4d5c88b into milvus-io:master Oct 25, 2024
4 checks passed
sre-ci-robot pushed a commit to milvus-io/pymilvus that referenced this pull request Oct 29, 2024
CaoHaiNam pushed a commit to CaoHaiNam/pymilvus that referenced this pull request Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants