-
Notifications
You must be signed in to change notification settings - Fork 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
Add dfs transformation function in XContentMapValues #17612
base: main
Are you sure you want to change the base?
Conversation
3d0c503
to
4617895
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17612 +/- ##
============================================
- Coverage 72.46% 72.37% -0.10%
+ Complexity 65757 65713 -44
============================================
Files 5311 5311
Lines 305001 305073 +72
Branches 44230 44243 +13
============================================
- Hits 221022 220796 -226
- Misses 65932 66116 +184
- Partials 18047 18161 +114 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
server/src/main/java/org/opensearch/common/xcontent/support/XContentMapValues.java
Outdated
Show resolved
Hide resolved
6e6b7ae
to
4e06dc5
Compare
❕ Gradle check result for 4e06dc5: UNSTABLE
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
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 @jmazanec15 , had one suggestion for an additional test case but otherwise LGTM
server/src/test/java/org/opensearch/common/xcontent/support/XContentMapValuesTests.java
Show resolved
Hide resolved
4e06dc5
to
f9b40ec
Compare
Thanks @jed326 - added the test |
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, thanks! @msfroh do you want to take another pass at this?
❌ Gradle check result for f9b40ec: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for f9b40ec: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for f9b40ec: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
I'm not sure what is actually failing here, @jmazanec15 maybe try rebasing your branch if the next retry fails again |
Adds a transformation function for XContentMapValues that performs depth first traversal into a map, potentially applying transformations to different values along the way. Main application for the method will be to provide masks that change values in the map without compromising the structure. Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
f9b40ec
to
f43e1d6
Compare
Sure @jed326 , just rebased |
❌ Gradle check result for f43e1d6: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for f43e1d6: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Description
In KNN, we are developing feature to remove vectors from source from documents before serialization. Originally, we were using the XContentMapValues.filter function to remove the vector fields from the source objects (ref). However, the problem with filter is that if a filter produces an empty object, then it will be removed from the array if its in an array. This alters the structure of the source map. Thus, putting the vectors back becomes very complex for nested cases (opensearch-project/k-NN#2583).
After some discussion, we are pivoting to an approach that instead of filtering the fields, we will mask the field with a smaller representation on write, and vice versa on read. I added PoC here: https://github.com/jmazanec15/k-NN-1/tree/mask-derived-poc. This change adds a transform method to XContentMapValues that performs depth first traversal on a map, potentially applying transformations to different values along the way. I could add in k-NN plugin, but thought it might be useful in future in core. The DFS nature works well with the architecture of nested documents.
Related Issues
Resolves opensearch-project/k-NN#2377
Check List
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.