Allow passing query between egress filter and filter gateway. #2097
Conversation
|
[CHATOPS:HELP] ChatOps commands.
|
kevindiu
force-pushed
the
feature/egress-gateway/add-egress-query
branch
from
f25941c
to
c05ffa7
Compare
Deploying vald with
|
| Latest commit: |
ad034dc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b3029982.vald.pages.dev |
| Branch Preview URL: | https://feature-egress-gateway-add-e.vald.pages.dev |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
dockers/example/client/gateway/filter/egress-filter/server/Dockerfile
Outdated
Show resolved
Hide resolved
dockers/example/client/gateway/filter/ingress-filter/server/Dockerfile
Outdated
Show resolved
Hide resolved
|
[WARNING:INTCFG] Changes in |
github-actions
bot
added
area/gateway/filter
area/gateway/lb
area/internal
area/makefile
area/manager/index
team/sre
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
LanguageTool
docs/user-guides/client-api-config.md|647 col 39| This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: Message
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
example/client/gateway/filter/README.md|8 col 4| This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: Gateway
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
example/client/gateway/filter/README.md|11 col 4| This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: Filter
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
example/client/gateway/filter/README.md|13 col 4| This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: Agent
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
example/client/gateway/filter/README.md|22 col 13| Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Suggestions: vald
Rule: https://community.languagetool.org/rule/show/ENGLISH_WORD_REPEAT_RULE?lang=en-US
Category: MISC
example/client/gateway/filter/README.md|47 col 61| This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: Go
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2097 +/- ##
==========================================
- Coverage 18.92% 18.57% -0.36%
==========================================
Files 565 567 +2
Lines 65223 66428 +1205
==========================================
- Hits 12346 12341 -5
- Misses 52081 53290 +1209
- Partials 796 797 +1 ☔ View full report in Codecov by Sentry. |
kevindiu
force-pushed
the
feature/egress-gateway/add-egress-query
branch
from
0d131ad
to
114385d
Compare
kevindiu
force-pushed
the
feature/egress-gateway/add-egress-query
branch
from
dfdb51d
to
21df055
Compare
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
WalkthroughWalkthroughThis update introduces new payload types and modifies existing ones to enhance filter functionality in the API documentation. It includes changes to the Changes
Tip New Features and ImprovementsReview SettingsIntroduced new personality profiles for code reviews. Users can now select between "Chill" and "Assertive" review tones to tailor feedback styles according to their preferences. The "Assertive" profile posts more comments and nitpicks the code more aggressively, while the "Chill" profile is more relaxed and posts fewer comments. AST-based InstructionsCodeRabbit offers customizing reviews based on the Abstract Syntax Tree (AST) pattern matching. Read more about AST-based instructions in the documentation. Community-driven AST-based RulesWe are kicking off a community-driven initiative to create and share AST-based rules. Users can now contribute their AST-based rules to detect security vulnerabilities, code smells, and anti-patterns. Please see the ast-grep-essentials repository for more information. New Static Analysis ToolsWe are continually expanding our support for static analysis tools. We have added support for Tone SettingsUsers can now customize CodeRabbit to review code in the style of their favorite characters or personalities. Here are some of our favorite examples:
Revamped Settings PageWe have redesigned the settings page for a more intuitive layout, enabling users to find and adjust settings quickly. This change was long overdue; it not only improves the user experience but also allows our development team to add more settings in the future with ease. Going forward, the changes to Miscellaneous
Tip Early Access Features
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Out of diff range and nitpick comments (9)
internal/client/v1/client/filter/egress/client.go (1)
Line range hint
153-168: Consider avoiding modification of input parameters directly as it can lead to side effects if the input object is reused.- in.Distance = res.GetDistance() + modifiedIn := *in + modifiedIn.Distance = res.GetDistance() + return modifiedIn, nilapis/swagger/v1/vald/upsert.swagger.json (1)
81-90: Add a more detailed description for theFilterQueryto enhance API documentation.apis/swagger/v1/vald/update.swagger.json (1)
81-90: Add a more detailed description for theFilterQueryto enhance API documentation.internal/config/filter_test.go (1)
Line range hint
31-66: Ensure that theDistanceFiltersandObjectFiltersare correctly initialized and used in the tests. Consider adding more comprehensive tests covering various scenarios.pkg/gateway/filter/handler/grpc/option_test.go (2)
Line range hint
712-787: Consider implementing the test cases forWithDistanceFilterTargets.It appears that the test cases for
WithDistanceFilterTargetsare outlined but not implemented. Would you like me to help by providing a sample implementation or opening a GitHub issue to track this task?
Line range hint
798-873: Consider implementing the test cases forWithObjectFilterTargets.Similar to
WithDistanceFilterTargets, the test cases forWithObjectFilterTargetsare also outlined but not implemented. If you need, I can assist by providing a sample implementation or creating a GitHub issue to ensure this is addressed.internal/client/v1/client/filter/egress/client_test.go (1)
Line range hint
544-565: Complete the implementation of test cases forFilterDistance.Please ensure to implement the test cases for
FilterDistanceas they are currently marked with TODO and are essential for validating the new functionality.pkg/gateway/filter/handler/grpc/handler.go (2)
58-59: Add documentation for new fieldsDistanceFiltersandObjectFilters.It would enhance maintainability and clarity if you add comments describing the purpose and usage of these new fields in the
serverstruct.
Line range hint
1437-1505: Refactor duplicate code in egress filter processing.- if filterConfigs != nil || s.DistanceFilters != nil { - filters := make([]*config.DistanceFilterConfig, 0, len(filterConfigs)+len(s.DistanceFilters)) - filters = append(filters, s.DistanceFilters...) - for _, c := range filterConfigs { - filters = append(filters, &config.DistanceFilterConfig{ - Addr: net.JoinHostPort(c.GetTarget().GetHost(), uint16(c.GetTarget().GetPort())), - Query: c.Query.GetQuery(), - }) - } - for _, filterConfig := range filters { - c, err := s.egress.Target(ctx, filterConfig.Addr) - if err != nil { - err = status.WrapWithUnavailable( - fmt.Sprintf(vald.SearchRPCName+" API egress filter target %v not found", filterConfig.Addr), - err, - &errdetails.RequestInfo{ - RequestId: req.GetConfig().GetRequestId(), - ServingData: errdetails.Serialize(req), - }, - &errdetails.BadRequest{ - FieldViolations: []*errdetails.BadRequestFieldViolation{ - { - Field: "vectorizer target", - Description: err.Error(), - }, - }, - }, - &errdetails.ResourceInfo{ - ResourceType: errdetails.ValdGRPCResourceTypePrefix + "/vald.v1." + vald.SearchRPCName, - ResourceName: fmt.Sprintf("%s: %s(%s)", apiName, s.name, s.ip), - }, info.Get()) - log.Warn(err) - if span != nil { - span.RecordError(err) - span.SetAttributes(trace.StatusCodeUnavailable(err.Error())...) - span.SetStatus(trace.StatusError, err.Error()) - } - return nil, err - } - dist := res.GetResults() - q := &payload.Filter_Query{ - Query: filterConfig.Query, - } - d, err := c.FilterDistance(ctx, &payload.Filter_DistanceRequest{ - Distance: dist, - Query: q, - }) - if err != nil { - err = status.WrapWithInternal( - fmt.Sprintf(vald.SearchRPCName+" API egress filter request to %v failure on distance %v and query %v", filterConfig.Addr, dist, q), - err, - &errdetails.RequestInfo{ - RequestId: req.GetConfig().GetRequestId(), - ServingData: errdetails.Serialize(req), - }, - &errdetails.ResourceInfo{ - ResourceType: errdetails.ValdGRPCResourceTypePrefix + "/vald.v1." + vald.SearchRPCName, - ResourceName: fmt.Sprintf("%s: %s(%s)", apiName, s.name, s.ip), - }, info.Get()) - log.Warn(err) - if span != nil { - span.RecordError(err) - span.SetAttributes(trace.StatusCodeInternal(err.Error())...) - span.SetStatus(trace.StatusError, err.Error()) - } - return nil, err - } - res.Results = d.GetDistance() - } - } + processEgressFilters(ctx, req, res, s, span)The handling of egress filters is duplicated in multiple places. Consider extracting this logic into a separate method to reduce duplication and improve maintainability.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (5)
apis/grpc/v1/filter/egress/egress_filter.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/filter/egress/egress_filter_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.json.gois excluded by!**/*.pb.json.goapis/grpc/v1/payload/payload_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (45)
- apis/docs/v1/docs.md (9 hunks)
- apis/proto/v1/filter/egress/egress_filter.proto (1 hunks)
- apis/proto/v1/payload/payload.proto (6 hunks)
- apis/swagger/v1/filter/egress/egress_filter.swagger.json (6 hunks)
- apis/swagger/v1/vald/filter.swagger.json (6 hunks)
- apis/swagger/v1/vald/insert.swagger.json (3 hunks)
- apis/swagger/v1/vald/object.swagger.json (2 hunks)
- apis/swagger/v1/vald/search.swagger.json (3 hunks)
- apis/swagger/v1/vald/update.swagger.json (3 hunks)
- apis/swagger/v1/vald/upsert.swagger.json (3 hunks)
- charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml (1 hunks)
- charts/vald-benchmark-operator/schemas/job-values.yaml (1 hunks)
- charts/vald-helm-operator/crds/valdrelease.yaml (1 hunks)
- charts/vald/values.schema.json (1 hunks)
- charts/vald/values.yaml (1 hunks)
- docs/overview/component/filter-gateway.md (2 hunks)
- docs/user-guides/client-api-config.md (15 hunks)
- docs/user-guides/filtering-configuration.md (1 hunks)
- example/client/gateway/filter/README.md (1 hunks)
- example/client/gateway/filter/egress-filter/main.go (1 hunks)
- example/client/gateway/filter/ingress-filter/main.go (1 hunks)
- example/client/gateway/filter/main.go (1 hunks)
- example/client/go.mod.default (2 hunks)
- example/client/mirror/main.go (2 hunks)
- example/manifest/filter/egress/Dockerfile (1 hunks)
- example/manifest/filter/egress/deployment.yaml (1 hunks)
- example/manifest/filter/egress/service.yaml (1 hunks)
- example/manifest/filter/ingress/Dockerfile (1 hunks)
- example/manifest/filter/ingress/deployment.yaml (1 hunks)
- example/manifest/filter/ingress/service.yaml (1 hunks)
- example/server/egress-filter/main.go (1 hunks)
- example/server/ingress-filter/main.go (1 hunks)
- internal/client/v1/client/filter/egress/client.go (10 hunks)
- internal/client/v1/client/filter/egress/client_test.go (8 hunks)
- internal/config/benchmark.go (2 hunks)
- internal/config/filter.go (3 hunks)
- internal/config/filter_test.go (6 hunks)
- pkg/gateway/filter/handler/grpc/handler.go (15 hunks)
- pkg/gateway/filter/handler/grpc/handler_test.go (42 hunks)
- pkg/gateway/filter/handler/grpc/option.go (2 hunks)
- pkg/gateway/filter/handler/grpc/option_test.go (8 hunks)
- pkg/gateway/filter/usecase/vald.go (1 hunks)
- pkg/tools/benchmark/job/service/object.go (2 hunks)
- rust/libs/proto/src/filter.egress.v1.tonic.rs (5 hunks)
- rust/libs/proto/src/payload.v1.rs (6 hunks)
Files not summarized due to errors (1)
- pkg/gateway/filter/handler/grpc/handler.go: Error: Message exceeds token limit
Files not reviewed due to errors (13)
- example/server/ingress-filter/main.go (no review received)
- example/client/gateway/filter/README.md (no review received)
- example/manifest/filter/ingress/Dockerfile (no review received)
- example/client/gateway/filter/ingress-filter/main.go (no review received)
- internal/config/filter.go (no review received)
- example/client/gateway/filter/egress-filter/main.go (no review received)
- pkg/gateway/filter/handler/grpc/option.go (no review received)
- pkg/tools/benchmark/job/service/object.go (no review received)
- docs/user-guides/filtering-configuration.md (no review received)
- docs/overview/component/filter-gateway.md (no review received)
- example/client/mirror/main.go (no review received)
- pkg/gateway/filter/usecase/vald.go (no review received)
- charts/vald-benchmark-operator/schemas/job-values.yaml (no review received)
Files skipped from review due to trivial changes (6)
- example/client/go.mod.default
- example/manifest/filter/egress/Dockerfile
- example/manifest/filter/egress/deployment.yaml
- example/manifest/filter/egress/service.yaml
- example/manifest/filter/ingress/deployment.yaml
- example/manifest/filter/ingress/service.yaml
Additional Context Used
Learnings (1)
pkg/gateway/filter/handler/grpc/handler.go (1)
User: kpango PR: vdaas/vald#2491 File: pkg/gateway/lb/handler/grpc/handler.go:306-316 Timestamp: 2024-05-07T04:33:44.775Z Learning: User: kpango PR: vdaas/vald#2491 File: pkg/gateway/lb/handler/grpc/handler.go:306-316 Timestamp: 2024-05-07T04:15:11.886Z Learning: In this project, vtprotobuf is used, which provides enhanced methods for protobuf objects: CloneVT (object cloning), EqualVT (object comparison), SizeVT (returns object size), MarshalVT (faster version of proto.Marshal), and UnmarshalVT (faster version of proto.Unmarshal). These methods offer better performance than usual protobuf usage.
Additional comments not posted (54)
apis/proto/v1/filter/egress/egress_filter.proto (2)
32-32: TheFilterDistanceRPC method is correctly defined with appropriate HTTP annotations.
40-40: TheFilterVectorRPC method is correctly defined with appropriate HTTP annotations.internal/client/v1/client/filter/egress/client.go (5)
Line range hint
133-144: Method implementation forFilterDistanceinclientstruct looks good.
Line range hint
177-192: Consider avoiding modification of input parameters directly as it can lead to side effects if the input object is reused.
Line range hint
201-212: Method implementation forFilterVectorinclientstruct looks good.
Line range hint
221-236: Consider avoiding modification of input parameters directly as it can lead to side effects if the input object is reused.
Line range hint
245-260: Consider avoiding modification of input parameters directly as it can lead to side effects if the input object is reused.apis/swagger/v1/filter/egress/egress_filter.swagger.json (2)
23-23: Ensure consistency in reference definitions in Swagger files.The reference
FilterDistanceResponseis used here but its definition is not provided within the visible scope of the file. Verify that it is defined elsewhere in the Swagger files or included in the definitions section of this file to ensure consistency and avoid broken references.Verification successful
The definition of
FilterDistanceResponsehas been confirmed to exist within the Swagger files. This addresses the concern raised in the review comment about ensuring the consistency of reference definitions in the Swagger documentation.
- The reference is used in
egress_filter.swagger.json.- It is defined in one of the Swagger JSON files, as indicated by the search results.
This verification confirms that the reference to
FilterDistanceResponseis consistent and correctly defined within the Swagger files.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of `FilterDistanceResponse` in Swagger files. # Test: Search for the definition in Swagger files. Expect: At least one occurrence. rg --type json '$"FilterDistanceResponse"' apis/swagger/**/*.jsonLength of output: 132
Script:
#!/bin/bash # First, confirm the existence and correct path of Swagger files in the repository. fd '.*\.json' apis/swagger # If files are found, search for the definition of `FilterDistanceResponse` in those files. fd '.*\.json' apis/swagger --exec rg 'FilterDistanceResponse' {}Length of output: 982
40-40: Validate the completeness of Swagger documentation.The reference
FilterDistanceRequestis crucial for understanding the API's capabilities. Ensure that this definition is complete and accurately documented in the Swagger files.Verification successful
The verification process has successfully identified the definitions of
FilterDistanceRequestandObjectDistancewithin the Swagger documentation. TheFilterDistanceRequestis defined as an object with properties including adistancearray ofObjectDistanceobjects and aqueryreference toFilterQuery. TheObjectDistanceis also well-defined with properties such asid,distance, andobject, each with appropriate types and descriptions.This confirms that the reference
FilterDistanceRequestis complete and accurately documented in the Swagger files, addressing the initial concern raised in the review comment.
- Location of
FilterDistanceRequestdefinition: Found in the Swagger JSON files underdefinitions.- Location of
ObjectDistancedefinition: Also located in the Swagger JSON files underdefinitions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of `FilterDistanceRequest` in Swagger files. # Test: Search for the definition in Swagger files. Expect: Complete and accurate documentation. rg --type json '$"FilterDistanceRequest"' apis/swagger/**/*.jsonLength of output: 131
Script:
#!/bin/bash # Correcting the directory error and searching for the definition of `FilterDistanceRequest` in Swagger files. fd '.*\.json' apis/swagger/ --exec rg '"FilterDistanceRequest"' {}Length of output: 101
Script:
#!/bin/bash # Retrieve the full content of the `FilterDistanceRequest` definition from the Swagger files. fd '.*\.json' apis/swagger/ --exec rg -A 10 '"FilterDistanceRequest"' {}Length of output: 358
Script:
#!/bin/bash # Verify the definition of `ObjectDistance` and check for additional properties in `FilterDistanceRequest`. fd '.*\.json' apis/swagger/ --exec rg -A 10 -B 10 '"ObjectDistance"' {} fd '.*\.json' apis/swagger/ --exec rg -A 20 -B 10 '"FilterDistanceRequest"' {}Length of output: 2493
apis/swagger/v1/vald/upsert.swagger.json (1)
233-237: Ensure that the array ofv1FilterConfigis correctly handled in the API implementation, especially in terms of serialization and deserialization.apis/swagger/v1/vald/update.swagger.json (2)
178-184: Ensure that the$reftov1FilterTargetis correctly defined elsewhere in the Swagger file or external references.Verification successful
The definition of
v1FilterTargetis indeed present in multiple Swagger files within the repository, including the specific file in question (apis/swagger/v1/vald/update.swagger.json). This confirms that the$reftov1FilterTargetis correctly defined and can be resolved within the Swagger documentation.
- File Locations:
apis/swagger/v1/vald/upsert.swagger.jsonapis/swagger/v1/vald/filter.swagger.jsonapis/swagger/v1/vald/search.swagger.jsonapis/swagger/v1/vald/insert.swagger.jsonapis/swagger/v1/vald/object.swagger.jsonapis/swagger/v1/vald/update.swagger.jsonScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of `v1FilterTarget` in Swagger files. # Test: Search for the definition of `v1FilterTarget`. Expect: At least one occurrence. rg --type json '$ref' | grep 'v1FilterTarget'Length of output: 45
Script:
#!/bin/bash # Description: Broaden the search for the definition of `v1FilterTarget` in all JSON files. # Test: Search all JSON files for 'v1FilterTarget' to ensure no occurrences are missed. rg 'v1FilterTarget' --type jsonLength of output: 1331
233-237: Ensure that the array ofv1FilterConfigis correctly handled in the API implementation, especially in terms of serialization and deserialization.rust/libs/proto/src/filter.egress.v1.tonic.rs (3)
107-112: LGTM! Thefilter_distancefunction is correctly implemented for the egress filter client.
138-143: LGTM! Thefilter_vectorfunction is correctly implemented for the egress filter client.
Line range hint
287-346: LGTM! The server implementations forfilter_distanceandfilter_vectorare correctly set up and match the client specifications.apis/proto/v1/payload/payload.proto (5)
145-149: LGTM! TheFilter.Querymessage is correctly defined with appropriate documentation.
160-171: LGTM! TheDistanceRequestandDistanceResponsemessages are correctly defined and documented.
174-184: LGTM! TheVectorRequestandVectorResponsemessages are correctly defined and include necessary fields for the new functionality.
90-92: LGTM! The changes toingress_filtersandegress_filtersinSearch.Configto allow multiple configurations are appropriate and well-documented.
154-157: LGTM! The modifications to theFilter.Configmessage, including the addition of thequeryfield, are correctly implemented and documented.apis/swagger/v1/vald/search.swagger.json (2)
273-282: LGTM! TheFilterQuerydefinition is correctly structured and well-documented in the Swagger JSON.
403-409: LGTM! The updates tov1FilterConfig, including the addition of thequeryfield, are correctly implemented and documented in the Swagger JSON.docs/user-guides/client-api-config.md (4)
69-75: TheFilter.Configstructure now includes aQueryfield. Ensure that all client code that constructsFilter.Configobjects is updated to handle this new field.
318-324: Ensure that theQueryfield is properly handled in all client code that constructsFilter.Configobjects for the Upsert service.
375-377: TheFilter.Confignow uses a singularTargetinstead of multiple targets for the Upsert service as well. Verify that all references toFilter.Configin the Upsert context have been updated accordingly.Verification successful
The structure of
Filter.Configand its usage withinUpsert_Confighave been verified to align with the updated requirements of using a singularTarget. Each instance ofFilter.Configin the list ofFilterswithinUpsert_Configcorrectly adheres to having a singularTarget.
- Location:
apis/grpc/v1/payload/payload.pb.golines 1430-1439 forFilter.Configstructure and lines 2515-2529 for its usage inUpsert_Config.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `Filter.Config` in the Upsert context have been updated to use a singular `Target`. # Test: Search for the usage of `Filter.Config` in the context of the Upsert service. Expect: Only singular `Target`. rg --type go 'Filter.Config'Length of output: 7388
Script:
#!/bin/bash # Description: Inspect the `Filter.Config` structure and its usage in `Upsert_Config`. # Check the definition of `Filter.Config` to see if it uses a singular `Target`. ast-grep --lang go --pattern $'type Filter_Config struct { $$$ Target $_ $$$ }' # Check the usage of `Filter.Config` within `Upsert_Config` to ensure it uses a singular `Target`. ast-grep --lang go --pattern $'type Upsert_Config struct { $$$ Filters []*Filter_Config $$$ }'Length of output: 2479
511-526: Ensure that theQueryfield is properly handled in all client code that constructsFilter.Configobjects for the Search service.Verification successful
The verification process has confirmed that the
Queryfield is properly handled in the client code that constructsFilter.Configobjects for the Search service, as evidenced by the explicit setting of this field inexample/client/gateway/filter/main.go. Based on the available evidence, it appears that the client code is updated to handle the newQueryfield correctly.
- File:
example/client/gateway/filter/main.go
- Details: The
Queryfield is explicitly set within theFilter_Configobject used in the context of the Search service.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all client code that constructs `Filter.Config` objects for the Search service is updated to handle the new `Query` field. # Test: Search for the construction of `Filter.Config` objects in the context of the Search service. Expect: Inclusion of the `Query` field. rg --type go 'Filter_Config'Length of output: 6428
Script:
#!/bin/bash # Description: Narrow down the search to instances where `Filter_Config` is used specifically in the context of the Search service and check for the handling of the `Query` field. # Search for the usage of `Filter_Config` in files related to the Search service and check if the `Query` field is being set or accessed. rg --type go -C 10 'Filter_Config' | grep -B 10 -A 10 'Search'Length of output: 9811
apis/swagger/v1/vald/filter.swagger.json (5)
273-282: The addition of theFilterQuerydefinition is well-formed and aligns with the PR's objective to enhance filter functionalities. The structure and description are clear and adhere to JSON schema standards.
417-423: The modification to thev1FilterConfigto include aqueryfield is correctly implemented. The reference to theFilterQuerydefinition ensures consistency and reusability of the schema.
451-455: The addition offiltersas an array ofv1FilterConfiginv1InsertConfigis a significant change that enhances the configurability of insert operations. This change supports the dynamic application of multiple filters, which is crucial for the new functionalities introduced.
547-559: The changes toingressFiltersandegressFiltersinv1SearchConfig, converting them to arrays ofv1FilterConfig, are well-executed. This modification supports the application of multiple filters in search operations, enhancing the system's flexibility and capability.
Line range hint
638-701: The modifications to thefiltersfield inv1UpdateConfigandv1UpsertConfig, making it an array ofv1FilterConfig, are correctly implemented. This change allows for multiple filter configurations during update and upsert operations, aligning with the enhanced filtering capabilities introduced in this PR.charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml (1)
329-333: Addition offilter_configsinobject_configlooks good.The use of
x-kubernetes-preserve-unknown-fields: trueis noted. While this allows for flexibility in configuration, please ensure that it is used cautiously to avoid unintended configurations being accepted.rust/libs/proto/src/payload.v1.rs (6)
102-106: The change to makeingress_filtersandegress_filtersrepeated fields is aligned with the PR's objective to enhance filtering capabilities. This allows for more complex configurations where multiple filters can be applied sequentially or in parallel.
211-218: The addition of theQuerystruct within theFiltermodule is crucial for supporting the new query functionalities as described in the PR. This struct will facilitate passing raw query strings to the filter system, enhancing its flexibility.
224-228: The modification to includetargetandqueryas optional fields in theConfigstruct enhances the configurability of filters. This change supports the PR's goal of enabling more complex filter setups, potentially involving multiple targets and queries.
233-248: The introduction ofDistanceRequestandDistanceResponsestructs is a significant enhancement for handling distance-based filtering operations. This aligns with the PR's enhancements to the filter functionalities, allowing for more specialized operations based on distance metrics.
252-266: The addition ofVectorRequestandVectorResponsestructs supports vector-based filtering operations. This is in line with the PR's objectives to enhance filtering capabilities, particularly for operations that involve vector data.
325-326: The change to makefiltersa repeated field in theConfigstructs ofInsert,Update, andUpsertmodules is a crucial enhancement. This allows for applying multiple filters in these operations, aligning with the PR's goal to enhance filtering capabilities across different operations.Also applies to: 388-389, 455-456
apis/docs/v1/docs.md (11)
16-21: Added new filter entities for handling distance and vector requests and responses.
211-215: UpdatedFilter.Configto includequeryfield. Ensure that the description of thequeryfield is clear and matches its intended use.
216-225: IntroducedFilter.DistanceRequestwith fields for distance and query. Verify that theObject.Distancetype is correctly used here and that thequeryfield is implemented as expected in the system.Verification successful
The verification of the
Filter.DistanceRequeststructure, specifically the usage ofObject.Distanceand the implementation of thequeryfield, has been successfully completed based on the provided search results:
- The
Object.Distancetype is used consistently across various parts of the system, including within theFilter.DistanceRequestas indicated in the API documentation. This confirms its correct usage in the specified context.- The
queryfield, linked toFilter.Query, is also well-integrated and used in various configurations and API endpoints, aligning with the expectations set in the documentation.Both components are implemented as described and expected in the system.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of Object.Distance in Filter.DistanceRequest and the implementation of the query field. # Test: Search for the usage of Object.Distance and query field in the system. rg --type md "Object.Distance" rg --type md "query"Length of output: 10849
227-235: AddedFilter.DistanceResponsewith a structure similar toFilter.DistanceRequest. Ensure consistency in the handling of distance data across requests and responses.
237-245: DefinedFilter.Queryentity. Ensure thequeryfield is utilized correctly across different components that interact with filters.Verification successful
The search results from the broadened query have provided a comprehensive list of references to
Filter.Queryacross various files in the codebase. The references are found in both Go and JavaScript files, indicating that theFilter.Queryentity is being utilized in the system's code, particularly within the gateway and API components.Here are the key points from the search results:
- The
Filter.Querystructure is used in multiple Go files (pkg/gateway/filter/handler/grpc/handler.go,pkg/tools/benchmark/job/service/object.go, etc.), where it is instantiated and manipulated.- The entity is also referenced in the API documentation (
apis/docs/v1/docs.md), which is consistent with the markdown documentation.- The protobuf definitions (
apis/grpc/v1/payload/payload.pb.go) includeFilter.Query, showing its integration in the system's data handling.Given these findings, it appears that the
Filter.Queryentity is actively used in the codebase, aligning with the documentation and the intended design. Therefore, the initial review comment concern about ensuring the correct utilization of thequeryfield across different components interacting with filters seems to be addressed adequately in the codebase.The `Filter.Query` entity is utilized correctly across various components in the codebase, including API handlers and configuration setups. Here are some of the key files where it is used: - `pkg/gateway/filter/handler/grpc/handler.go`: Handlers for gateway filtering using `Filter.Query`. - `pkg/tools/benchmark/job/service/object.go`: Benchmarking tools that configure and use `Filter.Query`. - `apis/grpc/v1/payload/payload.pb.go`: Protobuf definitions that include `Filter.Query` for API interactions. This confirms that the `query` field is integrated and used as expected within the system's architecture.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct utilization of the `query` field in components interacting with filters. # Test: Search for the usage of the `query` field in the system. rg --type md "query"Length of output: 6495
Script:
#!/bin/bash # Description: Search for the usage of `Filter.Query` and its `query` field in the actual source code files to verify correct utilization. # Search for references to `Filter.Query` and its `query` field in JavaScript files. rg --type js "Filter.Query"Length of output: 176
Script:
#!/bin/bash # Description: Broaden the search for the usage of `Filter.Query` and its `query` field to include all file types, ensuring no files are unintentionally skipped. # Search for references to `Filter.Query` in all file types, ensuring no files are skipped. rg "Filter.Query" --debugLength of output: 83654
258-267: IntroducedFilter.VectorRequestwith fields for vector and query. Verify that theObject.Vectortype is correctly used here and that thequeryfield is implemented as expected in the system.Verification successful
The verification of the
Object.Vectortype and thequeryfield in the context ofFilter.VectorRequesthas been successfully completed based on the search results from the documentation and code references:
- The
Object.Vectortype is used correctly as a field type forvectorinFilter.VectorRequest.- The
queryfield is implemented as expected, with its type beingFilter.Queryand described appropriately in the system's documentation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of Object.Vector in Filter.VectorRequest and the implementation of the query field. # Test: Search for the usage of Object.Vector and query field in the system. rg --type md "Object.Vector" rg --type md "query"Length of output: 16048
269-277: AddedFilter.VectorResponsewith a structure similar toFilter.VectorRequest. Ensure consistency in the handling of vector data across requests and responses.
511-515: UpdatedInsert.Configto include repeatedFilter.Configfor filters and a new field for timestamp. Ensure that these changes are reflected in the corresponding implementation and that the repeated nature of filters is handled correctly.Verification successful
The review comment's concerns about the implementation of the repeated
Filter.Configand the newtimestampfield inInsert.Confighave been addressed. The search results confirm that these configurations are extensively documented and referenced across various parts of the system, indicating a thorough integration.
- The repeated
Filter.Configis used in multiple API endpoints and is well-documented in user guides and API documentation.- The
timestampfield is also widely mentioned, with clear documentation on its usage in insertion, update, and removal operations.This comprehensive implementation suggests that the changes are correctly reflected in the system as per the updates in the
Insert.Config.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of repeated Filter.Config in Insert.Config and the new timestamp field. # Test: Search for the usage of Filter.Config and timestamp in Insert.Config across the system. rg --type md "Filter.Config" rg --type md "timestamp"Length of output: 26231
856-867: UpdatedSearch.Configto include repeated fields for ingress and egress filters, and new fields formin_numandaggregation_algorithm. Verify that these fields are utilized correctly in search operations.Verification successful
The fields
ingress_filters,egress_filters,min_num, andaggregation_algorithmhave been thoroughly integrated and are utilized correctly in search operations across the system. Here are the specific findings:
- Ingress and Egress Filters: Both fields are consistently mentioned in API and user guide documents, indicating their active use in configuring search operations.
- Minimum Number of Results (
min_num): This field is well-documented in various contexts, clearly explaining its role in specifying the minimum number of search results, which aligns with its intended purpose.- Aggregation Algorithm: The field is mentioned across multiple API documents with details on default values and options, showing its active use in configuring how search results are aggregated.
These findings confirm that the new fields in
Search.Configare utilized as intended.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct utilization of new fields in Search.Config. # Test: Search for the usage of ingress_filters, egress_filters, min_num, and aggregation_algorithm in the system. rg --type md "ingress_filters" rg --type md "egress_filters" rg --type md "min_num" rg --type md "aggregation_algorithm"Length of output: 19186
977-982: UpdatedUpdate.Configto include repeatedFilter.Configfor filters, a new field for timestamp, and a flag for disabling balanced update. Ensure these changes are correctly implemented and that the flag's behavior is documented.Verification successful
The fields
Filter.Config,timestamp, anddisable_balanced_updatehave been successfully verified in theUpdate.Configacross the system. Each field is implemented and documented as described:
Filter.Configis used as a repeated field in various configurations, includingUpdate.Config.- The
timestampfield is consistently implemented and documented across insert, update, and remove operations.- The
disable_balanced_updateflag is present and described in the context of update operations, specifically detailing its role in disabling balanced updates.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of repeated Filter.Config in Update.Config, the new timestamp field, and the disable_balanced_update flag. # Test: Search for the usage of Filter.Config, timestamp, and disable_balanced_update in Update.Config across the system. rg --type md "Filter.Config" rg --type md "timestamp" rg --type md "disable_balanced_update"Length of output: 29732
1039-1044: UpdatedUpsert.Configto include repeatedFilter.Configfor filters, a new field for timestamp, and a flag for disabling balanced update. Verify these changes are correctly implemented and that the flag's behavior is documented.Verification successful
The changes to
Upsert.Configincluding the repeatedFilter.Configfor filters, the new field for timestamp, and the flag for disabling balanced update have been successfully verified across multiple documents in the codebase. These updates are correctly implemented as described.
- Filter.Config: Found in various
Upsert.Configsections across the documentation.- Timestamp: Included in
Upsert.Configand described in multiple documents.- Disable Balanced Update Flag: Documented in the context of
Upsert.Configin several files.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of repeated Filter.Config in Upsert.Config, the new timestamp field, and the disable_balanced_update flag. # Test: Search for the usage of Filter.Config, timestamp, and disable_balanced_update in Upsert.Config across the system. rg --type md "Filter.Config" rg --type md "timestamp" rg --type md "disable_balanced_update"Length of output: 29732
charts/vald/values.yaml (2)
1427-1427: Ensure theobject_filtersarray is correctly utilized in the system.
1430-1430: Check the integration and usage ofdistance_filtersin the system.Verification successful
The search results indicate that
distance_filtersis well-integrated and utilized across various parts of the system. It is defined in Go configuration structures, used in Helm chart values for user configuration, and conditionally applied in Helm templates for generating Kubernetes config maps. This setup suggests a robust integration allowing for flexible user configurations and system behavior adjustments based on those configurations.
- Go Configuration: Defined in
internal/config/filter.goas part of a struct, which likely means it's used programmatically within the system.- Helm Charts: Mentioned in
charts/vald/values.yamland used in templates likecharts/vald/templates/gateway/filter/configmap.yaml, indicating its role in user-driven configuration and deployment.- Custom Resource Definitions (CRDs): Appears in
k8s/operator/helm/crds/valdrelease.yamlandcharts/vald-helm-operator/crds/valdrelease.yaml, which are crucial for defining how the system's resources are structured and managed in Kubernetes environments.Based on this analysis, the integration and usage of
distance_filtersin the system are verified as being correctly implemented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the integration and usage of `distance_filters` in the system. # Test: Search for the usage of `distance_filters` in the codebase. Expect: Proper handling and usage in relevant parts of the system. rg --type yaml --type go 'distance_filters'Length of output: 1212
pkg/gateway/filter/handler/grpc/handler_test.go (1)
125-126: The addition ofDistanceFiltersandObjectFiltersas slices of pointers aligns with the PR's objectives to enhance filtering capabilities.Ensure that the code handling these configurations properly checks for nil pointers to avoid runtime panics.
Verification successful
The review of the nil pointer dereference handling for
DistanceFiltersandObjectFiltersin the codebase shows that appropriate nil checks are in place before their usage. This is a good practice to prevent runtime errors and aligns with the PR's objectives to enhance filtering capabilities safely.
- File:
pkg/gateway/filter/handler/grpc/handler.go
- Proper nil checks are performed before using
DistanceFiltersandObjectFilters.- File:
pkg/gateway/filter/usecase/vald.go
DistanceFiltersare checked for nil before usage.- File:
internal/config/filter.go
DistanceFiltersare checked for nil before usage.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for nil pointer dereference handling in the usage of DistanceFilters and ObjectFilters. # Test: Search for nil checks around the usage of DistanceFilters and ObjectFilters. rg --type go 'if \S+ != nil' --context 5 | grep -E 'DistanceFilters|ObjectFilters'Length of output: 1884
charts/vald-helm-operator/crds/valdrelease.yaml (2)
3879-3880: The configuration fordistance_filtersas an array of objects with preserved unknown fields is appropriate for dynamic filter configurations.
3884-3885: The configuration forobject_filtersmirrors that ofdistance_filters, ensuring consistency in handling various filter types.
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/dockers-ci-container-image.yml (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/dockers-ci-container-image.yml
This reverts commit ffecb7e.
| dimension uint | ||
| ) | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
don't use init function (init)
| Port option specifies grpc server port of your egress filter. Default value is `8083`. | ||
| Dimension option specifies dimension size of vectors. Default value is `784`. | ||
| **/ | ||
| flag.UintVar(&egressServerPort, "port", 8083, "server port") |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Magic number: 8083, in detected (gomnd)
| Dimension option specifies dimension size of vectors. Default value is `784`. | ||
| **/ | ||
| flag.UintVar(&egressServerPort, "port", 8083, "server port") | ||
| flag.UintVar(&dimension, "dimension", 784, "dimension size of vectors") |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Magic number: 784, in detected (gomnd)
| Port option specifies grpc server port of your ingress filter. Default value is `8082`. | ||
| Dimension option specifies dimension size of vectors. Default value is `784`. | ||
| **/ | ||
| flag.UintVar(&ingressServerPort, "port", 8082, "server port") |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Magic number: 8082, in detected (gomnd)
| Dimension option specifies dimension size of vectors. Default value is `784`. | ||
| **/ | ||
| flag.UintVar(&ingressServerPort, "port", 8082, "server port") | ||
| flag.UintVar(&dimension, "dimension", 784, "dimension size of vectors") |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Magic number: 784, in detected (gomnd)
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (1)
example/server/egress-filter/main.go (1)
95-101: Clarify the intended functionality of theFilterVectormethod or implement the filtering logic.The current implementation of
FilterVectorsimply logs the input and returns it unchanged. If this is the intended behavior, consider adding a comment to clarify that this is a placeholder or example implementation. Otherwise, implement the actual filtering logic based on the requirements.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- example/server/egress-filter/main.go (1 hunks)
Additional comments not posted (2)
example/server/egress-filter/main.go (2)
48-55: ThegetSplitValuefunction is implemented correctly.
57-59: The embedding ofUnimplementedFilterServerinmyEgressServeris a good practice.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Hiroto Funakoshi <hiroto.funakoshi.hiroto@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Hiroto Funakoshi <hiroto.funakoshi.hiroto@gmail.com>
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
| dimension uint | ||
| ) | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
don't use init function (init)
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- example/client/gateway/filter/main.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- example/client/gateway/filter/main.go
| Port option specifies grpc server port of your egress filter. Default value is `8083`. | ||
| Dimension option specifies dimension size of vectors. Default value is `784`. | ||
| **/ | ||
| flag.UintVar(&egressServerPort, "port", 8083, "server port") |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Magic number: 8083, in detected (gomnd)
| Dimension option specifies dimension size of vectors. Default value is `784`. | ||
| **/ | ||
| flag.UintVar(&egressServerPort, "port", 8083, "server port") | ||
| flag.UintVar(&dimension, "dimension", 784, "dimension size of vectors") |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Magic number: 784, in detected (gomnd)
| Port option specifies grpc server port of your egress filter. Default value is `8083`. | ||
| Dimension option specifies dimension size of vectors. Default value is `784`. | ||
| **/ | ||
| flag.UintVar(&egressServerPort, "port", 8083, "server port") |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Magic number: 8083, in detected (gomnd)
| Dimension option specifies dimension size of vectors. Default value is `784`. | ||
| **/ | ||
| flag.UintVar(&egressServerPort, "port", 8083, "server port") | ||
| flag.UintVar(&dimension, "dimension", 784, "dimension size of vectors") |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
Magic number: 784, in detected (gomnd)
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
| @@ -111,10 +112,18 @@ message Filter { | |||
| uint32 port = 2; | |||
| } | |||
|
|
|||
| // Represent the filter query. | |||
| message Query { | |||
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: Message
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
| // Represent the filter query. | ||
| message Query { | ||
| // The raw query string. | ||
| string query = 1; |
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: String
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
| @@ -235,10 +247,18 @@ | |||
| uint32 port = 2; | |||
| } | |||
|
|
|||
| // Represent the filter query. | |||
| message Query { | |||
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: Message
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
| // Represent the filter query. | ||
| message Query { | ||
| // The raw query string. | ||
| string query = 1; |
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: String
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
| @@ -358,10 +381,18 @@ | |||
| uint32 port = 2; | |||
| } | |||
|
|
|||
| // Represent the filter query. | |||
| message Query { | |||
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: Message
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
| // Represent the filter query. | ||
| message Query { | ||
| // The raw query string. | ||
| string query = 1; |
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: String
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
| @@ -576,10 +609,18 @@ | |||
| uint32 port = 2; | |||
| } | |||
|
|
|||
| // Represent the filter query. | |||
| message Query { | |||
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: Message
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
| // Represent the filter query. | ||
| message Query { | ||
| // The raw query string. | ||
| string query = 1; |
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: String
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
| @@ -601,10 +642,18 @@ | |||
| uint32 port = 2; | |||
| } | |||
|
|
|||
| // Represent the filter query. | |||
| message Query { | |||
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: Message
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
| // Represent the filter query. | ||
| message Query { | ||
| // The raw query string. | ||
| string query = 1; |
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
This sentence does not start with an uppercase letter. (UPPERCASE_SENTENCE_START)
Suggestions: String
URL: https://languagetool.org/insights/post/spelling-capital-letters/
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (3)
example/server/egress-filter/main.go (1)
92-98: Clarify the filtering logic inFilterVector.The comment "// Write your own logic" is too vague and does not provide clear guidance on what should be implemented. It would be beneficial to provide more detailed comments or examples of expected logic to help future developers understand the intended functionality.
docs/user-guides/client-api-config.md (2)
116-119: Clarify the expected format of thequerystring in theQuerymessage.The documentation for the
queryfield in theQuerymessage could be enhanced by specifying the expected format or constraints of the query string. This would help users understand how to construct their queries correctly.
251-255: Enhance the description of thequeryfield in theQuerymessage for theUpdateservice.The description of the
queryfield could be improved by providing examples or more detailed information on how it is used within the context of theUpdateservice. This would make the documentation more informative and useful for developers.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- docs/user-guides/client-api-config.md (15 hunks)
- example/server/egress-filter/main.go (1 hunks)
Additional Context Used
LanguageTool (6)
docs/user-guides/client-api-config.md (6)
Near line 480: Specify a number, remove phrase, or simply use “many” or “numerous”
Context: ...ance when asynchronous aggregation from a large number of Agents and a huge number of threads. ...
Rule ID: LARGE_NUMBER_OF
Near line 573: Loose punctuation mark.
Context: ...positive integer. #### radiusradius, the specific parameter for NGT, specifi...
Rule ID: UNLIKELY_OPENING_PUNCTUATION
Near line 576: To elevate your writing, try using a synonym here.
Context: ...etween accuracy and search speed. It is hard to set it depending on the dataset in m...
Rule ID: HARD_TO
Near line 576: The phrase ‘in many cases’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others.
Context: ...hard to set it depending on the dataset in many cases. The default value is infinity. When s...
Rule ID: IN_MANY_STYLE_CASES
Near line 587: Loose punctuation mark.
Context: ...process. #### epsilonepsilon, the specific parameter for NGT, specifi...
Rule ID: UNLIKELY_OPENING_PUNCTUATION
Near line 593: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ... desirable to adjust this value within 0 - 0.3, it can also set a negative value (o...
Rule ID: DASH_RULE
Markdownlint (216)
docs/user-guides/client-api-config.md (216)
36: Column: 1
Hard tabs
37: Column: 1
Hard tabs
39: Column: 1
Hard tabs
40: Column: 1
Hard tabs
41: Column: 1
Hard tabs
45: Column: 1
Hard tabs
46: Column: 1
Hard tabs
47: Column: 1
Hard tabs
48: Column: 1
Hard tabs
49: Column: 1
Hard tabs
50: Column: 1
Hard tabs
51: Column: 1
Hard tabs
52: Column: 1
Hard tabs
53: Column: 1
Hard tabs
54: Column: 1
Hard tabs
56: Column: 1
Hard tabs
57: Column: 1
Hard tabs
58: Column: 1
Hard tabs
59: Column: 1
Hard tabs
60: Column: 1
Hard tabs
61: Column: 1
Hard tabs
62: Column: 1
Hard tabs
63: Column: 1
Hard tabs
64: Column: 1
Hard tabs
65: Column: 1
Hard tabs
66: Column: 1
Hard tabs
67: Column: 1
Hard tabs
68: Column: 1
Hard tabs
69: Column: 1
Hard tabs
70: Column: 1
Hard tabs
71: Column: 1
Hard tabs
72: Column: 1
Hard tabs
73: Column: 1
Hard tabs
74: Column: 1
Hard tabs
75: Column: 1
Hard tabs
76: Column: 1
Hard tabs
77: Column: 1
Hard tabs
78: Column: 1
Hard tabs
79: Column: 1
Hard tabs
80: Column: 1
Hard tabs
81: Column: 1
Hard tabs
82: Column: 1
Hard tabs
83: Column: 1
Hard tabs
84: Column: 1
Hard tabs
163: Column: 1
Hard tabs
164: Column: 1
Hard tabs
166: Column: 1
Hard tabs
167: Column: 1
Hard tabs
168: Column: 1
Hard tabs
172: Column: 1
Hard tabs
173: Column: 1
Hard tabs
174: Column: 1
Hard tabs
175: Column: 1
Hard tabs
176: Column: 1
Hard tabs
177: Column: 1
Hard tabs
178: Column: 1
Hard tabs
179: Column: 1
Hard tabs
180: Column: 1
Hard tabs
181: Column: 1
Hard tabs
183: Column: 1
Hard tabs
184: Column: 1
Hard tabs
185: Column: 1
Hard tabs
186: Column: 1
Hard tabs
187: Column: 1
Hard tabs
188: Column: 1
Hard tabs
189: Column: 1
Hard tabs
191: Column: 1
Hard tabs
192: Column: 1
Hard tabs
193: Column: 1
Hard tabs
194: Column: 1
Hard tabs
195: Column: 1
Hard tabs
196: Column: 1
Hard tabs
197: Column: 1
Hard tabs
198: Column: 1
Hard tabs
199: Column: 1
Hard tabs
200: Column: 1
Hard tabs
201: Column: 1
Hard tabs
202: Column: 1
Hard tabs
203: Column: 1
Hard tabs
204: Column: 1
Hard tabs
205: Column: 1
Hard tabs
206: Column: 1
Hard tabs
207: Column: 1
Hard tabs
208: Column: 1
Hard tabs
209: Column: 1
Hard tabs
210: Column: 1
Hard tabs
211: Column: 1
Hard tabs
298: Column: 1
Hard tabs
299: Column: 1
Hard tabs
301: Column: 1
Hard tabs
302: Column: 1
Hard tabs
303: Column: 1
Hard tabs
307: Column: 1
Hard tabs
308: Column: 1
Hard tabs
309: Column: 1
Hard tabs
310: Column: 1
Hard tabs
311: Column: 1
Hard tabs
312: Column: 1
Hard tabs
313: Column: 1
Hard tabs
314: Column: 1
Hard tabs
315: Column: 1
Hard tabs
316: Column: 1
Hard tabs
318: Column: 1
Hard tabs
319: Column: 1
Hard tabs
320: Column: 1
Hard tabs
321: Column: 1
Hard tabs
322: Column: 1
Hard tabs
323: Column: 1
Hard tabs
324: Column: 1
Hard tabs
325: Column: 1
Hard tabs
326: Column: 1
Hard tabs
327: Column: 1
Hard tabs
328: Column: 1
Hard tabs
329: Column: 1
Hard tabs
330: Column: 1
Hard tabs
331: Column: 1
Hard tabs
332: Column: 1
Hard tabs
333: Column: 1
Hard tabs
334: Column: 1
Hard tabs
335: Column: 1
Hard tabs
336: Column: 1
Hard tabs
337: Column: 1
Hard tabs
338: Column: 1
Hard tabs
339: Column: 1
Hard tabs
340: Column: 1
Hard tabs
341: Column: 1
Hard tabs
342: Column: 1
Hard tabs
343: Column: 1
Hard tabs
344: Column: 1
Hard tabs
491: Column: 1
Hard tabs
492: Column: 1
Hard tabs
494: Column: 1
Hard tabs
495: Column: 1
Hard tabs
496: Column: 1
Hard tabs
500: Column: 1
Hard tabs
501: Column: 1
Hard tabs
502: Column: 1
Hard tabs
503: Column: 1
Hard tabs
504: Column: 1
Hard tabs
505: Column: 1
Hard tabs
506: Column: 1
Hard tabs
507: Column: 1
Hard tabs
508: Column: 1
Hard tabs
509: Column: 1
Hard tabs
511: Column: 1
Hard tabs
512: Column: 1
Hard tabs
513: Column: 1
Hard tabs
514: Column: 1
Hard tabs
515: Column: 1
Hard tabs
516: Column: 1
Hard tabs
517: Column: 1
Hard tabs
518: Column: 1
Hard tabs
519: Column: 1
Hard tabs
520: Column: 1
Hard tabs
521: Column: 1
Hard tabs
522: Column: 1
Hard tabs
523: Column: 1
Hard tabs
524: Column: 1
Hard tabs
525: Column: 1
Hard tabs
526: Column: 1
Hard tabs
527: Column: 1
Hard tabs
528: Column: 1
Hard tabs
529: Column: 1
Hard tabs
530: Column: 1
Hard tabs
531: Column: 1
Hard tabs
532: Column: 1
Hard tabs
533: Column: 1
Hard tabs
534: Column: 1
Hard tabs
535: Column: 1
Hard tabs
536: Column: 1
Hard tabs
537: Column: 1
Hard tabs
538: Column: 1
Hard tabs
539: Column: 1
Hard tabs
540: Column: 1
Hard tabs
541: Column: 1
Hard tabs
542: Column: 1
Hard tabs
543: Column: 1
Hard tabs
544: Column: 1
Hard tabs
545: Column: 1
Hard tabs
546: Column: 1
Hard tabs
548: Column: 1
Hard tabs
549: Column: 1
Hard tabs
551: Column: 1
Hard tabs
552: Column: 1
Hard tabs
553: Column: 1
Hard tabs
554: Column: 1
Hard tabs
692: Column: 1
Hard tabs
693: Column: 1
Hard tabs
695: Column: 1
Hard tabs
696: Column: 1
Hard tabs
697: Column: 1
Hard tabs
701: Column: 1
Hard tabs
702: Column: 1
Hard tabs
703: Column: 1
Hard tabs
704: Column: 1
Hard tabs
705: Column: 1
Hard tabs
706: Column: 1
Hard tabs
707: Column: 1
Hard tabs
708: Column: 1
Hard tabs
709: Column: 1
Hard tabs
710: Column: 1
Hard tabs
712: Column: 1
Hard tabs
713: Column: 1
Hard tabs
714: Column: 1
Hard tabs
716: Column: 1
Hard tabs
717: Column: 1
Hard tabs
718: Column: 1
Hard tabs
719: Column: 1
Hard tabs
720: Column: 1
Hard tabs
721: Column: 1
Hard tabs
722: Column: 1
Hard tabs
723: Column: 1
Hard tabs
724: Column: 1
Hard tabs
725: Column: 1
Hard tabs
726: Column: 1
Hard tabs
727: Column: 1
Hard tabs
Additional comments not posted (3)
docs/user-guides/client-api-config.md (3)
385-389: Add error handling forFilter_Queryinitialization in theUpsertservice.- Query: &payload.Filter_Query{}, + Query: &payload.Filter_Query{Query: "example upsert query"},Similar to previous services, the
Filter_Queryin theUpsertservice is initialized but not populated. Providing a default or example query would demonstrate its usage more effectively.Likely invalid or redundant comment.
529-544: Verify the integration ofFilter_Queryin theSearchservice.The example code includes
Filter_Querybut does not show how it integrates with the rest of theSearchservice. Verifying its usage and integration can help ensure that it functions as expected within this context.
196-202: Consider handling potential errors fromFilter_Queryinitialization.The example code initializes
Filter_Querybut does not demonstrate how to handle potential errors that might arise during this process. It would be beneficial to include error handling to ensure robustness.
| dimension uint | ||
| ) | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
Avoid using the init function for setting global variables.
Consider refactoring the init function to avoid global state mutation and improve testability. Using an init function to parse flags and set global variables can lead to issues with testability and clarity. Consider moving this logic to the main function or another explicitly called setup function. This approach not only makes the code easier to understand but also enhances its maintainability by making the dependencies and side effects of your configuration more explicit.
| glg.Fatal(err) | ||
| } | ||
|
|
||
| s := grpc.NewServer() |
There was a problem hiding this comment.
Secure the gRPC server to prevent potential security risks.
- s := grpc.NewServer()
+ s := grpc.NewServer(grpc.Creds(credentials.NewServerTLSFromFile("cert.pem", "cert.key")))Ensure that the server uses TLS credentials to establish a secure connection. This change is crucial to prevent unauthorized access and data tampering.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| s := grpc.NewServer() | |
| s := grpc.NewServer(grpc.Creds(credentials.NewServerTLSFromFile("cert.pem", "cert.key"))) |
| func (s *myEgressServer) FilterDistance(ctx context.Context, in *payload.Filter_DistanceRequest) (*payload.Filter_DistanceResponse, error) { | ||
| glg.Log("filtering vector %#v", in) | ||
| qCategory, ok := getSplitValue(in.GetQuery().GetQuery(), "=", 1) | ||
| if !ok { | ||
| return &payload.Filter_DistanceResponse{ | ||
| Distance: in.GetDistance(), | ||
| }, nil | ||
| } | ||
|
|
||
| filteredDis := []*payload.Object_Distance{} | ||
| for _, d := range in.GetDistance() { | ||
| iCategory, ok := getSplitValue(d.GetId(), "_", 1) | ||
| if !ok { | ||
| continue | ||
| } | ||
| glg.Infof("qCategory: %v, iCategory: %v", qCategory, iCategory) | ||
| if qCategory == iCategory { | ||
| filteredDis = append(filteredDis, d) | ||
| } | ||
| } | ||
|
|
||
| if len(filteredDis) == 0 { | ||
| return nil, status.Error(codes.NotFound, "FilterDistance results not found.") | ||
| } | ||
|
|
||
| return &payload.Filter_DistanceResponse{ | ||
| Distance: filteredDis, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Refactor the FilterDistance method to improve error handling and logic clarity.
func (s *myEgressServer) FilterDistance(ctx context.Context, in *payload.Filter_DistanceRequest) (*payload.Filter_DistanceResponse, error) {
glg.Log("filtering vector %#v", in)
qCategory, ok := getSplitValue(in.GetQuery().GetQuery(), "=", 1)
if !ok {
return nil, status.Error(codes.InvalidArgument, "Invalid query format")
}
var filteredDis []*payload.Object_Distance
for _, d := range in.GetDistance() {
iCategory, ok := getSplitValue(d.GetId(), "_", 1)
if !ok {
continue
}
if qCategory == iCategory {
filteredDis = append(filteredDis, d)
}
}
if len(filteredDis) == 0 {
return nil, status.Error(codes.NotFound, "FilterDistance results not found.")
}
return &payload.Filter_DistanceResponse{
Distance: filteredDis,
}, nil
}This refactoring introduces clearer error messages and avoids unnecessary nil checks by directly returning errors. It also simplifies the loop by continuing early when conditions are not met.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (s *myEgressServer) FilterDistance(ctx context.Context, in *payload.Filter_DistanceRequest) (*payload.Filter_DistanceResponse, error) { | |
| glg.Log("filtering vector %#v", in) | |
| qCategory, ok := getSplitValue(in.GetQuery().GetQuery(), "=", 1) | |
| if !ok { | |
| return &payload.Filter_DistanceResponse{ | |
| Distance: in.GetDistance(), | |
| }, nil | |
| } | |
| filteredDis := []*payload.Object_Distance{} | |
| for _, d := range in.GetDistance() { | |
| iCategory, ok := getSplitValue(d.GetId(), "_", 1) | |
| if !ok { | |
| continue | |
| } | |
| glg.Infof("qCategory: %v, iCategory: %v", qCategory, iCategory) | |
| if qCategory == iCategory { | |
| filteredDis = append(filteredDis, d) | |
| } | |
| } | |
| if len(filteredDis) == 0 { | |
| return nil, status.Error(codes.NotFound, "FilterDistance results not found.") | |
| } | |
| return &payload.Filter_DistanceResponse{ | |
| Distance: filteredDis, | |
| }, nil | |
| } | |
| func (s *myEgressServer) FilterDistance(ctx context.Context, in *payload.Filter_DistanceRequest) (*payload.Filter_DistanceResponse, error) { | |
| glg.Log("filtering vector %#v", in) | |
| qCategory, ok := getSplitValue(in.GetQuery().GetQuery(), "=", 1) | |
| if !ok { | |
| return nil, status.Error(codes.InvalidArgument, "Invalid query format") | |
| } | |
| var filteredDis []*payload.Object_Distance | |
| for _, d := range in.GetDistance() { | |
| iCategory, ok := getSplitValue(d.GetId(), "_", 1) | |
| if !ok { | |
| continue | |
| } | |
| if qCategory == iCategory { | |
| filteredDis = append(filteredDis, d) | |
| } | |
| } | |
| if len(filteredDis) == 0 { | |
| return nil, status.Error(codes.NotFound, "FilterDistance results not found.") | |
| } | |
| return &payload.Filter_DistanceResponse{ | |
| Distance: filteredDis, | |
| }, nil | |
| } |
| // Represent insert configurations. | ||
| message Config { | ||
| // Check whether or not the same set of vector and ID is already inserted. | ||
| // A flag to skip exist check during insert operation. | ||
| bool skip_strict_exist_check = 1; | ||
| // Configuration for filters if your Vald cluster uses filters. | ||
| Filter.Config filters = 2; | ||
| // The timestamp when the vector was inserted. | ||
| // Filter configurations. | ||
| repeated Filter.Config filters = 2; | ||
| // Insert timestamp. |
There was a problem hiding this comment.
Consider adding a description for the filters field in the Config message.
The filters field lacks a detailed description in the Config message for the Insert service. Would you like me to draft a description for this field to enhance the documentation?
| Filters: []*payload.Filter_Config{ | ||
| { | ||
| Target: &payload.Filter_Target{ | ||
| Host: "vald-ingress-filter", | ||
| Port: 8081, | ||
| }, | ||
| Query: &payload.Filter_Query{}, |
There was a problem hiding this comment.
Ensure proper initialization of Filter_Query in the example code.
- Query: &payload.Filter_Query{},
+ Query: &payload.Filter_Query{Query: "example query"},The Filter_Query object is initialized but not populated with any query string. It's a good practice to provide a meaningful default or example query to demonstrate its intended use.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| Filters: []*payload.Filter_Config{ | |
| { | |
| Target: &payload.Filter_Target{ | |
| Host: "vald-ingress-filter", | |
| Port: 8081, | |
| }, | |
| Query: &payload.Filter_Query{}, | |
| Filters: []*payload.Filter_Config{ | |
| { | |
| Target: &payload.Filter_Target{ | |
| Host: "vald-ingress-filter", | |
| Port: 8081, | |
| }, | |
| Query: &payload.Filter_Query{Query: "example query"}, |
Description:
This PR updated the following changes:
queryto egress filter throughFilterDistanceandFilterVectorAPIapis/proto/v1/filter/egress/egress_filter.protopkg/gateway/filter/handler/grpc/handler.goexample/client/gateway/filter/README.mdRelated Issue:
Versions:
Checklist:
Special notes for your reviewer:
You can check the behavior after this modification by reading the
example/client/gateway/filter/README.md.You may also need to edit the below file to enable filter gateway and setting the image tag to validate the implementation.
Please do not merge this PR, unless the target merge branch is updated.
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation
Chores