-
Notifications
You must be signed in to change notification settings - Fork 41
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
Create runtime fields #622
Conversation
5d4d3ee
to
80b3f22
Compare
src/main/java/com/yelp/nrtsearch/server/luceneserver/SearchHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yelp/nrtsearch/server/luceneserver/SearchHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yelp/nrtsearch/server/luceneserver/search/SearchRequestProcessor.java
Outdated
Show resolved
Hide resolved
* @param docLookup index level doc value lookup provider | ||
* @return {@link ValueSource} to evaluate script | ||
*/ | ||
ValueSource newFactory(Map<String, Object> params, DocLookup docLookup); |
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.
I'm not sure you are really getting much using ValueSource
. SegmentFactory
could be a simple interface.
The RuntimeFieldDef
could just provide a RuntimeScript.SegmentFactory
implementation.
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. Converting to SegmentFactory was much simpler and more straightforward.
src/main/java/com/yelp/nrtsearch/server/luceneserver/script/RuntimeScript.java
Outdated
Show resolved
Hide resolved
compositeFieldValue.addFieldValue( | ||
SearchResponse.Hit.FieldValue.newBuilder().setTextValue(String.valueOf(obj))); | ||
} | ||
if (obj instanceof Long) { |
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.
You will probably need to add some more types here. Converting a Map
into a Struct
would be an important one.
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.
I tried to do map, but got some errors converting a map to proto object.
I added a TODO to add map and list.
88fe12c
to
fca27c0
Compare
@aprudhomme thank you for reviewing this. I applied your suggestions. Can you please take a look at this to see if the overall design is good enough so that I can create tests for this PR? I will try to support map and list types with the test commit . |
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 looks like a reasonable approach
c8ff362
to
767903d
Compare
9e4dca0
to
86eda24
Compare
@aprudhomme can you please take a look at this? I think the failing tests are flaky ones that don't have anything to do with my changes. I |
1521755
to
8a04251
Compare
@@ -598,12 +608,30 @@ message SearchResponse { | |||
google.protobuf.Struct structValue = 8; // Value for structured data | |||
// Value for VECTOR FieldType | |||
Vector vectorValue = 9; | |||
RepeatedFieldValues repeatedFieldValues = 10; |
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.
Would it be easier to use the existing ListValue
type https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/struct.proto#L92
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 for the pointer. I spent a lot of time creating this and its builder methods. Using ListValue
and the existing struct builders would have saved me a lot of time, if I had seen them earlier.
@@ -763,7 +768,6 @@ private CompositeFieldValue getFieldForHit( | |||
|
|||
// We detect invalid field above: | |||
assert fd != null; | |||
|
|||
if (fd instanceof VirtualFieldDef) { |
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.
Does the runtime field also need to be covered in this code path?
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.
Somehow the code never comes here when I put a breakpoint in this line. I assume it's not necessary to do that anymore. It doesn't stop here, even when I have a virtual field in the request.
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 code path is used for doing parallel fetch by chunks of fields, instead of documents. It should probably be supported.
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.
Do you know how I can test that locally?
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 the config used for the current testing https://github.com/Yelp/nrtsearch/blob/master/src/test/java/com/yelp/nrtsearch/server/grpc/MultiSegmentParallelFieldsFetchTest.java#L25
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. I was able to debug this part of the code with the above configs. It now returns the same object as the one with fetching docs.
src/main/java/com/yelp/nrtsearch/server/luceneserver/SearchHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yelp/nrtsearch/server/luceneserver/SearchHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yelp/nrtsearch/server/luceneserver/SearchHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yelp/nrtsearch/server/luceneserver/script/RuntimeScript.java
Outdated
Show resolved
Hide resolved
src/test/java/com/yelp/nrtsearch/server/luceneserver/facet/RuntimeScriptFacetsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/yelp/nrtsearch/server/luceneserver/facet/RuntimeScriptFacetsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/yelp/nrtsearch/server/luceneserver/facet/RuntimeScriptFacetsTest.java
Outdated
Show resolved
Hide resolved
@@ -91,11 +91,14 @@ public static UpdatedFieldInfo updateFields( | |||
FieldAndFacetState.Builder fieldStateBuilder = currentState.toBuilder(); | |||
List<Field> nonVirtualFields = new ArrayList<>(); | |||
List<Field> virtualFields = new ArrayList<>(); | |||
List<Field> runtimeFields = new ArrayList<>(); |
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.
Is this used anymore?
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.
No, it wasn't used. So I removed it.
4f3bb8b
to
5dd576a
Compare
@aprudhomme can you take a look at this when you get a chance? |
clientlib/src/main/java/com/yelp/nrtsearch/server/utils/StructJsonUtils.java
Outdated
Show resolved
Hide resolved
@@ -763,7 +768,6 @@ private CompositeFieldValue getFieldForHit( | |||
|
|||
// We detect invalid field above: | |||
assert fd != null; | |||
|
|||
if (fd instanceof VirtualFieldDef) { |
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 code path is used for doing parallel fetch by chunks of fields, instead of documents. It should probably be supported.
The following query works now. It supports all the numeric values, strings, maps and lists.
Which gave me the following: