-
Notifications
You must be signed in to change notification settings - Fork 16
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 support for default exclusion of $vector and $vectorize #1016
Conversation
@@ -191,36 +228,32 @@ static PathCollector collectPaths(JsonNode def, boolean includeSimilarity) { | |||
} | |||
|
|||
public DocumentProjector buildProjector() { | |||
if (isDefaultProjection()) { |
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.
Caller will use the short-cut, no need to check whether explicit definition happens to work like default projection.
@@ -126,8 +126,7 @@ public void byIdAfterUpdate() { | |||
"name": "Joe", | |||
"age": 42, | |||
"enabled": true, | |||
"value": -1, | |||
"$vector" : [ 0.5, -0.25 ] |
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.
Simplifying the test since $vector not really needed for test (but new default projection would drop)
…avoid dropping $vector accidentally
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 so far
paths, | ||
slices, | ||
// doc-id included unless explicitly excluded | ||
!Boolean.FALSE.equals(idInclusion), |
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.
This is interesting.
So by default, idInclusion will be null, which will give addDocId as
- true for inclusion-based projection
- false for exclusion-based projection
So essentially the meaning of addDocId will be flipped, in terms of what projection is used inclusion-based
or exclusion-based
. The variable name addDocId (similarly add$vector, add$vectorize) gives some confusion to me initially, but it makes sense afterwards.
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.
Good point, naming is hard. Instead of "addDocId" could use "includeDocId" but that would be even more confusing.
If you (or anyone else) has better name suggestion would be happy to rename.
if we understand correctly, |
Correct: |
What this PR does:
Will change default projection inclusion to exclude
$vector
and$vectorize
, needing explicit inclusion.Which issue(s) this PR fixes:
Fixes #1005
Checklist