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

Add filter function for NeuralQueryBuilder and HybridQueryBuilder and… #1206

Merged
merged 1 commit into from
Mar 19, 2025

Conversation

chloewqg
Copy link
Contributor

@chloewqg chloewqg commented Mar 3, 2025

… modify fromXContent function in HybridQueryBuilder to support filter field.

Description

Add filter function to NeuralQueryBuilder and HybridQueryBuilder, which allows to push down non null filter. One exception is that when HybridQueryBuilder has a nested HybridQueryBuilder, then calling the filter function will cause UnsupportedOperationException

Related Issues

Resolves #1206
Related: #282, #1135

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

if (!validateFilterParams(filter)) {
return this;
}
HybridQueryBuilder compoundQueryBuilder = new HybridQueryBuilder();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have to create a new query builder instead of returning this?

Copy link
Member

Choose a reason for hiding this comment

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

I cannot find validateFilterParams method. Besides that the naming is confusing, if you conclude validity of the object based on return value that needs to be in the name, something like isValidFilterParams. Also change negation to an explicit == false

Copy link
Collaborator

Choose a reason for hiding this comment

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

The method is added in core. opensearch-project/OpenSearch#17409
Should we have to use that method? Can we just do null check explicitly here instead of relying on the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi the validateFilterParams is in the core package, inside the AbstractQueryBuilder class. https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/index/query/AbstractQueryBuilder.java#L94

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we have to create a new query builder instead of returning this?

Because I saw the the queries of HybridQueryBuilder is marked as final. https://github.com/opensearch-project/neural-search/blob/main/src/main/java/org/opensearch/neuralsearch/query/HybridQueryBuilder.java#L56

So I think as we need to reassign a list of queries to the queries variable inside the HybridQueryBuilder. So we can only create a new HybridQueryBuilder to solve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't this double the filters sizes? Like if there are two filters, when we add(query.filter(filter)), wouldn't this create duplicate filters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Martin for the comment! Actually the function name validateFilterParams was suggested by Daniel in the pull request of core package. opensearch-project/OpenSearch#17409 , is it okay to leave as what it is for now?

if method is already there let's utilize it. Please use == false syntax instead of negation, I see you've used it in other places in your PR

Yep. Updated in the newer commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quote reply

I think the filter input check is common across all the query builders. Although current check is simple of whether it equals to null, adding a function here can allow complicated logic changes in the future

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 you can do something like queries.set(i, queries.get(i).filter(filter));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha!

Copy link
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

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

there is a separate issue and the whole design/RFC for hybrid query. Is this a building block for that design or this is a conflicting issue?
Ref: #282, #1135

if (!validateFilterParams(filter)) {
return this;
}
HybridQueryBuilder compoundQueryBuilder = new HybridQueryBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

I cannot find validateFilterParams method. Besides that the naming is confusing, if you conclude validity of the object based on return value that needs to be in the name, something like isValidFilterParams. Also change negation to an explicit == false

@chloewqg
Copy link
Contributor Author

chloewqg commented Mar 4, 2025

there is a separate issue and the whole design/RFC for hybrid query. Is this a building block for that design or this is a conflicting issue? Ref: #282, #1135

Hi Martin, this is a building block for that design. cc @bzhangam

@martin-gaievski
Copy link
Member

there is a separate issue and the whole design/RFC for hybrid query. Is this a building block for that design or this is a conflicting issue? Ref: #282, #1135

Hi Martin, this is a building block for that design. cc @bzhangam

Got it, makes sense then. Can you please mention same in the PR description and also update the link to the feature request issue under "Related issues"? If the filtering feature requires multiple PRs/building blocks I suggest you create a high-level meta issue and host smaller PRs under that meta issue.

Copy link

codecov bot commented Mar 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.83%. Comparing base (57124dd) to head (f9b9425).
Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##               main    #1206     +/-   ##
===========================================
  Coverage     81.83%   81.83%             
+ Complexity     2607     1311   -1296     
===========================================
  Files           190       95     -95     
  Lines          8922     4481   -4441     
  Branches       1520      764    -756     
===========================================
- Hits           7301     3667   -3634     
+ Misses         1028      518    -510     
+ Partials        593      296    -297     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chloewqg
Copy link
Contributor Author

chloewqg commented Mar 4, 2025

there is a separate issue and the whole design/RFC for hybrid query. Is this a building block for that design or this is a conflicting issue? Ref: #282, #1135

Hi Martin, this is a building block for that design. cc @bzhangam

Got it, makes sense then. Can you please mention same in the PR description and also update the link to the feature request issue under "Related issues"? If the filtering feature requires multiple PRs/building blocks I suggest you create a high-level meta issue and host smaller PRs under that meta issue.

Gotcha. Thanks!

* }
*/
@SneakyThrows
public void testQueryWithBoostAndFilterApplied() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we add this IT? I think the filter function is an existing feature for neural query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we are testing whether adding another filter on NeuralQuery would be function as expected.

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 IT is mock a real use case. If we want to test that we should test a hybrid query with a neural sub query. And both the hybrid query and the neural query have a filter.

In this test we simply test the filter function of the neural query builder which has already be done as a unit test.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine to have one end-to-end test with neural query and some complex filter, but I would remove the "match none" part. All the permutations and edge cases should be tested with UT, integ test are quite expensive for this.

}
}*/
@SneakyThrows
private void testRangeQueryAsFilter(String indexName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we add this IT in PostFilterIT? Filter is different from post filter. I think we should either add it to the HybridQueryIT or create a new HybridQueryFilterIT.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you plan to raise another PR to add tests for BWC? BWC tests are under the qa folder.

Copy link
Member

Choose a reason for hiding this comment

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

Why we add this IT in PostFilterIT? Filter is different from post filter. I think we should either add it to the HybridQueryIT or create a new HybridQueryFilterIT.

+1, better add new class HybridQueryFilterIT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you plan to raise another PR to add tests for BWC? BWC tests are under the qa folder.

I think the current bwc tests is blocking the merge. So I will do the bwc tests here in the same PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why we add this IT in PostFilterIT? Filter is different from post filter. I think we should either add it to the HybridQueryIT or create a new HybridQueryFilterIT.

Sure. Will create a new HybridQueryFilterIT

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw you created HybridQueryFilterIT. Do you plan to remove this test?

if (validateFilterParams(filter) == false) {
return this;
}
for (int i = 0; i < queries.size(); i++) {
Copy link
Collaborator

@heemin32 heemin32 Mar 10, 2025

Choose a reason for hiding this comment

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

This is fine because the queries are array list. However, we should avoid the assumption that it will be ArrayList especially because we defined as List.

private final List<QueryBuilder> queries = new ArrayList<>();

Therefore, following is better so that its performance does not degrade even if we switch to other list type. Also, we don't need to check if query is type of HybridQuery or not. It should be checked somewhere else.

ListIterator<QueryBuilder> iterator = queries.listIterator();
while(iterator.hasNext()) {
  QueryBuilder query = iterator.next();
  // set the query again because query.filter(filter) can return new query.
  iterator.set(query.filter(filter));
}

// set the query again because query.filter(filter) can return new query. This comment is important because query.filter(filter) can return a new query builder instance, so we need to reassign it. Additionally, we should have a unit test to cover this case to ensure the test fails if the query builder is not updated. In fact, the query.filter() method is somewhat misleadingly named, as it suggests that it simply applies a filter to the existing query builder rather than creating a new query builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha! Thanks Heemin! Btw, I am little confused in the current bwc tests failure. Seems like the error message is

Caused by: io.netty.channel.AbstractChannel$AnnotatedConnectException: Connection refused: 127.0.0.1/127.0.0.1:42235
»  Caused by: java.net.ConnectException: Connection refused

I am bit confused on what is wrong here with my filter function changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should have a unit test to cover this case to ensure the test fails if the query builder is not updated

For this case, the last test case in testFilter() function cover such scenario

ListIterator<QueryBuilder> iterator = queries.listIterator();
while(iterator.hasNext()) {
QueryBuilder query = iterator.next();
if (query instanceof HybridQueryBuilder) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Heemin, thanks for your comments. Do you mean check of hybrid query build type? I remember in design doc that when the filter query is a hybrid query and we should throw unsupported operation for it.

Copy link
Collaborator

@heemin32 heemin32 Mar 12, 2025

Choose a reason for hiding this comment

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

Copy link
Collaborator

@heemin32 heemin32 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

/**
* Function to support filter on HybridQueryBuilder filter. Currently pushing down a filter
* to HybridQueryBuilder is not supported by design. We would simply check if the filter is valid
* and throw exception telling this is an unsupported operation. If the filter is null, then we do nothing and
Copy link
Contributor

Choose a reason for hiding this comment

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

The description doesn't match the implementation. Have we changed the mind that we want to support the filter push down for a hybrid query? Basically it means we want to support a hybrid query nested in another hybrid query. I don't think we plan to support it in near future.

Copy link
Collaborator

@heemin32 heemin32 Mar 14, 2025

Choose a reason for hiding this comment

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

I asked to remove the check of nested hybrid query here. I believe the blocking of the nested hybrid query should be handled elsewhere, but it doesn’t necessarily have to be in this specific location. https://github.com/opensearch-project/neural-search/pull/498/files#diff-64ab60781cf1e9f7963a415406f49283b26945beaa46b7042e7cc5044d8d95b1R168

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still I think we should update the comment which does not match with the actual implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Please state in the comment that filter is not stored in the top hybrid query but only pushed down to individual sub-queries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Will publish new revision for that.

@@ -51,6 +52,7 @@ public final class HybridQueryBuilder extends AbstractQueryBuilder<HybridQueryBu
public static final String NAME = "hybrid";

private static final ParseField QUERIES_FIELD = new ParseField("queries");
private static final ParseField FILTER_FIELD = new ParseField("filter");
Copy link
Contributor

Choose a reason for hiding this comment

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

The new field should also be added to HybridQueryBuilder(StreamInput in) and doXContent(XContentBuilder builder, Params params) so that we will lose the filter info when we pass it across nodes. Even though the filter info is already included in the sub queries I think we still should follow the best practice to include it in those two functions.

Besides we also want to add it to the doEquals and doHashCode function in case they are used to compare two hybrid queries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like we are missing some test case here unless this filter setting is happening in coordination node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Bo, quick question while I am trying to follow your comments.

The new field should also be added to HybridQueryBuilder(StreamInput in) and doXContent(XContentBuilder builder, Params params) so that we will lose the filter info.

What do you mean by we will lose the filter info? Could you please give an example?

add it to the doEquals and doHashCode function in case they are used to compare two hybrid queries.

The field I added here is not the filter itself but the filter parser field name. I believe the filter itself is pushed down to queries. So we are automatically comparing the queries and its filter in doEquals and doHashCode. What do you mean by adding it to the doEquals and doHashCode function?

Copy link
Contributor

Choose a reason for hiding this comment

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

For your first question when we pass the query across nodes we will convert it to a stream and later in another node build it based on the stream. If we don't clearly write the filter to the stream we will lose it.

For the second question we override the doEquals and doHashCode function so if we don't clearly check it those two functions will ignore it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Synced offline. Since currently we always want to push down the filter to sub-queries and we always do this work in the fromXContent function there is no need to persist the filter in the HybridQuery. In this case there is not need to write it to the stream since it's already included in the sub-queries. Same for the doEqual and doHash functions.

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be fine, one concern I do have is some edge case in context of cluster upgrade. Can you add a bwc test, it may be skipped for now for 2.x -> 3.x migration, but it will run in future for next 3.x, e.g. 3.0 -> 3.1. You can check how it's done in of the recent PR for stats API

}
}*/
@SneakyThrows
private void testRangeQueryAsFilter(String indexName) {
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 most common use case for hybrid query is using a neural query and a normal term query as sub queries. Can we build the IT for that use case?

One important thing we want to validate is that for neural query we should push the filter into it. It can give us different result compared to the case we use a bool query to combine a filter with a neural query. This happens because the filter in neural query will be applied before we return the top k result while the filter combined through a bool query will happen after returning the top k result. Would recommend to build a test case for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, sounds good

Copy link
Member

@vibrantvarun vibrantvarun left a comment

Choose a reason for hiding this comment

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

  1. Please add bwc tests as this field is added new to the query. We need to evaluate the impact in case of bwc environment.
  2. Why we have not added this field in HybridQuery.java?

cc: @martin-gaievski @heemin32

@heemin32
Copy link
Collaborator

heemin32 commented Mar 14, 2025

  1. Why we have not added this field in HybridQuery.java?

We just pass the filter down to sub queries. Therefore, there is no need for hybrid query to hold it as we are not going to filter after the sub queries return the result.

while (iterator.hasNext()) {
QueryBuilder query = iterator.next();
// set the query again because query.filter(filter) can return new query.
iterator.set(query.filter(filter));
Copy link
Member

Choose a reason for hiding this comment

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

we can save few CPU cycles here if we check for a queryWithFilter ref, if that's same as query then there is no need in calling set() method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good

Copy link
Collaborator

Choose a reason for hiding this comment

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

@martin-gaievski, If you're okay with it, let's keep this as is. The difference between equals and set is negligible, so a simpler approach would be preferable.

Copy link
Member

Choose a reason for hiding this comment

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

ok, let's keep the set() method, I agree that original version of the code is simpler

@@ -74,6 +74,13 @@ public void testPostFilterOnIndexWithSingleShard_whenConcurrentSearchEnabled_the
testPostFilterMatchAllAndMatchNoneQueries(TEST_MULTI_DOC_INDEX_WITH_TEXT_AND_INT_SINGLE_SHARD);
}

@SneakyThrows
public void testFilterOnIndexWithSingleShard_whenConcurrentSearchEnabled_thenSuccessful() {
Copy link
Member

Choose a reason for hiding this comment

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

how exactly this test is related to post-filter? You've created a new IT just for filter in neural and hybrid, I think this method should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes will remove it.

@@ -763,6 +768,29 @@ private NeuralQueryBuilder getBaselineNeuralQueryBuilder() {
.build();
}

public void testFilter() {
Copy link
Member

Choose a reason for hiding this comment

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

please follow method name convention - "testFeature_whenScenario_thenExpectedOutcome"

@@ -176,6 +177,101 @@ public void testQueryWithBoostAndImageQueryAndRadialQuery() {
);
}

/**
* Tests basic query without filter:
Copy link
Member

Choose a reason for hiding this comment

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

this is tested in existing tests, can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will remove the test here.

* }
*/
@SneakyThrows
public void testQueryWithBoostAndFilterApplied() {
Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine to have one end-to-end test with neural query and some complex filter, but I would remove the "match none" part. All the permutations and edge cases should be tested with UT, integ test are quite expensive for this.

@@ -51,6 +52,7 @@ public final class HybridQueryBuilder extends AbstractQueryBuilder<HybridQueryBu
public static final String NAME = "hybrid";

private static final ParseField QUERIES_FIELD = new ParseField("queries");
private static final ParseField FILTER_FIELD = new ParseField("filter");
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be fine, one concern I do have is some edge case in context of cluster upgrade. Can you add a bwc test, it may be skipped for now for 2.x -> 3.x migration, but it will run in future for next 3.x, e.g. 3.0 -> 3.1. You can check how it's done in of the recent PR for stats API

/**
* Function to support filter on HybridQueryBuilder filter. Currently pushing down a filter
* to HybridQueryBuilder is not supported by design. We would simply check if the filter is valid
* and throw exception telling this is an unsupported operation. If the filter is null, then we do nothing and
Copy link
Member

Choose a reason for hiding this comment

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

Please state in the comment that filter is not stored in the top hybrid query but only pushed down to individual sub-queries

Comment on lines 321 to 326
if (filter == null) {
filter = filterToBeAdded;
return this;
}
filter = filter.filter(filterToBeAdded);
return this;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (filter == null) {
filter = filterToBeAdded;
return this;
}
filter = filter.filter(filterToBeAdded);
return this;
if (filter == null) {
filter = filterToBeAdded;
} else {
filter = filter.filter(filterToBeAdded);
}
return this;

while (iterator.hasNext()) {
final QueryBuilder query = iterator.next();
final QueryBuilder queryWithFilter = query.filter(filter);
if (!queryWithFilter.equals(query)) {
Copy link
Member

Choose a reason for hiding this comment

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

don't use equals method, you need to compare exact reference, like queryWithFilter == query. Also use == false instead of negation in if condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@martin-gaievski I don’t see any benefit from this change. The previous code was much better—using a set isn’t costly, even compared to an equals check. The earlier implementation was also simpler and more readable. Would it be possible to revert to the previous version?

By the way, in this code, we are calling query.filter(filter); two times.

modelId,
Map.of("ef_search", 100),
RescoreContext.getDefault(),
new MatchQueryBuilder("_id", "1")
Copy link
Member

Choose a reason for hiding this comment

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

why this change is needed, it doesn't look like a simple adoption of the new argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I checked previous PR and found they modify this for bwc tests. Shall I discard this change?

Copy link
Member

@martin-gaievski martin-gaievski Mar 18, 2025

Choose a reason for hiding this comment

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

if by default filter can be null then pass it as null. With current logic you're testing multiple things, but the test has been built just for rescore functionality

}

@SneakyThrows
public void testFilterOnIndexWithSingleShard_whenConcurrentSearchEnabled_thenSuccessful() {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this match of tests for CSS, one should be enough, this one you can delete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay!

}

@SneakyThrows
private void testNeuralQueryBuilder(String indexName) {
Copy link
Member

Choose a reason for hiding this comment

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

you need yo set the CSS setting explicitly, if index is same then setting can be on or off dpending on the order of execution, in other words it's not guaranteed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by set the CSS setting explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another question for [Build and Test Neural Search / Pre-commit Linux (23, ubuntu-latest) ] Failure. It is caused by java.lang.NoClassDefFoundError: Could not initialize class org.opensearch.knn.index.engine.KNNEngine Yesterday it works fine offline. What might be causing this issue?

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by set the CSS setting explicitly?

if you want CSS to be disabled for the test, make sure to explicitly set it to false. It looks like you're enabling it in tests where you need it, and assuming that it will be reset to false before each next test. It's not the case if tests are sharing same index. In other words your test may be running against index where CSS is enabled when it's not required

Copy link
Member

Choose a reason for hiding this comment

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

Another question for [Build and Test Neural Search / Pre-commit Linux (23, ubuntu-latest) ] Failure. It is caused by java.lang.NoClassDefFoundError: Could not initialize class org.opensearch.knn.index.engine.KNNEngine Yesterday it works fine offline. What might be causing this issue?

which run do you mean? I checked few failed ones at random, they are all flaky, model haven't loaded in time:

java.lang.AssertionError: failed to load the model, last task finished with status RUNNING

Copy link
Member

Choose a reason for hiding this comment

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

Another failure related to some recent changes in core, I think it's covered by this issue: #1232

… modify fromXContent function in HybridQueryBuilder to support filter field.

Signed-off-by: Chloe Gao <[email protected]>
@heemin32 heemin32 merged commit 8abb418 into opensearch-project:main Mar 19, 2025
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants