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.

Sign up for GitHub

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

Update pagination_depth datatype from int to Integer #1094

Merged
merged 3 commits into from
Jan 14, 2025
Merged

Update pagination_depth datatype from int to Integer #1094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c029ab
Update pagination_depth datatype from int to Integer
vibrantvarun Jan 14, 2025
0eb2641
Adding log of pagination_depth and correcting is null check
vibrantvarun Jan 14, 2025
c7d863c
Add TODO comment
vibrantvarun Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/main/java/org/opensearch/neuralsearch/query/HybridQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ public HybridQuery(final Collection<Query> subQueries, final List<Query> filterQ
if (subQueries.isEmpty()) {
throw new IllegalArgumentException("collection of queries must not be empty");
}
if (hybridQueryContext.getPaginationDepth() == 0) {
Integer paginationDepth = hybridQueryContext.getPaginationDepth();
if (Objects.nonNull(paginationDepth) && paginationDepth == 0) {
throw new IllegalArgumentException("pagination_depth must not be zero");
}
if (Objects.isNull(filterQueries) || filterQueries.isEmpty()) {
Expand Down
14 changes: 9 additions & 5 deletions src/main/java/org/opensearch/neuralsearch/query/HybridQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public final class HybridQueryBuilder extends AbstractQueryBuilder<HybridQueryBu

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

private int paginationDepth;
private Integer paginationDepth;

static final int MAX_NUMBER_OF_SUB_QUERIES = 5;
private final static int DEFAULT_PAGINATION_DEPTH = 10;
Expand All @@ -65,7 +65,7 @@ public HybridQueryBuilder(StreamInput in) throws IOException {
super(in);
queries.addAll(readQueries(in));
if (isClusterOnOrAfterMinReqVersionForPaginationInHybridQuery()) {
paginationDepth = in.readInt();
paginationDepth = in.readOptionalInt();
}
}

Expand All @@ -78,7 +78,7 @@ public HybridQueryBuilder(StreamInput in) throws IOException {
protected void doWriteTo(StreamOutput out) throws IOException {
writeQueries(out, queries);
if (isClusterOnOrAfterMinReqVersionForPaginationInHybridQuery()) {
out.writeInt(paginationDepth);
out.writeOptionalInt(paginationDepth);
Copy link
Member

@ryanbogan ryanbogan Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I ran into a similar error as this with the StreamOutput on k-NN. Using out.getVersion() to see what version you're writing to might help avoid null checks elsewhere. StreamInput has a similar method too so you can set a default for the value.

Example: https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/indices/ModelMetadata.java#L502

Copy link
Member Author

Choose a reason for hiding this comment

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

I will address this in #1097

}
}

Expand Down Expand Up @@ -109,8 +109,9 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
queryBuilder.toXContent(builder, params);
}
builder.endArray();
if (isClusterOnOrAfterMinReqVersionForPaginationInHybridQuery()) {
Copy link
Member

Choose a reason for hiding this comment

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

do we still need method isClusterOnOrAfterMinReqVersionForPaginationInHybridQuery in MinClusterVersionUtil? Looks like you're using isClusterOnOrAfterMinReqVersionForPaginationInHybridQuery == false and paginationDepth == null interchangeably

Copy link
Member Author

Choose a reason for hiding this comment

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

So how this works is older versions of OS when there is no pagination_depth sent then we will not set that field in the builder object. But when the same method runs for version>=2.19 then in the fromXContent method we give default value 10 when we don't get pagination_depth by the user.

Copy link
Member

Choose a reason for hiding this comment

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

in this case it's a combination of the version check and values being not null. Should you do OR of two? What I'm essentially trying to say - we are checking one condition in one place and another condition in another scenario, and it's not that great.

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is we do not use isClusterOnOrAfterMinReqVersionForPaginationInHybridQuery in doXContent of any other query builder. Every other place we use the same non null check. Recently expandNested param has been added by heemin in the neuralquerybuilder and it also follow the same non null check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will address this in #1097

builder.field(PAGINATION_DEPTH_FIELD.getPreferredName(), paginationDepth == 0 ? DEFAULT_PAGINATION_DEPTH : paginationDepth);
// TODO https://github.com/opensearch-project/neural-search/issues/1097
if (Objects.nonNull(paginationDepth)) {
builder.field(PAGINATION_DEPTH_FIELD.getPreferredName(), paginationDepth);
}
printBoostAndQueryName(builder);
builder.endObject();
Expand Down Expand Up @@ -324,6 +325,9 @@ private Collection<Query> toQueries(Collection<QueryBuilder> queryBuilders, Quer
}

private static void validatePaginationDepth(final int paginationDepth, final QueryShardContext queryShardContext) {
if (Objects.isNull(paginationDepth)) {
return;
}
if (paginationDepth < LOWER_BOUND_OF_PAGINATION_DEPTH) {
throw new IllegalArgumentException(
String.format(Locale.ROOT, "pagination_depth should be greater than %s", LOWER_BOUND_OF_PAGINATION_DEPTH)
Expand Down
4 changes: 1 addition & 3 deletions src/main/java/org/opensearch/neuralsearch/query/HybridQueryContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,12 @@

import lombok.Builder;
import lombok.Getter;
import lombok.NonNull;

/**
* Class that holds the low level information of hybrid query in the form of context
*/
@Builder
@Getter
public class HybridQueryContext {
@NonNull
private int paginationDepth;
private Integer paginationDepth;
}
1 change: 1 addition & 0 deletions src/main/java/org/opensearch/neuralsearch/search/query/HybridCollectorManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,7 @@ private static int getSubqueryResultsRetrievalSize(final SearchContext searchCon
if (searchContext.from() == 0) {
return searchContext.size();
}
log.info("pagination_depth is {}", paginationDepth);
return paginationDepth;
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/opensearch/neuralsearch/query/HybridQueryBuilderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ public void testFromXContent_whenMultipleSubQueries_thenBuildSuccessfully() {
assertEquals(2, queryTwoSubQueries.queries().size());
assertTrue(queryTwoSubQueries.queries().get(0) instanceof NeuralQueryBuilder);
assertTrue(queryTwoSubQueries.queries().get(1) instanceof TermQueryBuilder);
assertEquals(10, queryTwoSubQueries.paginationDepth());
assertEquals(10, queryTwoSubQueries.paginationDepth().intValue());
// verify knn vector query
NeuralQueryBuilder neuralQueryBuilder = (NeuralQueryBuilder) queryTwoSubQueries.queries().get(0);
assertEquals(VECTOR_FIELD_NAME, neuralQueryBuilder.fieldName());
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/opensearch/neuralsearch/query/HybridQueryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public void testQueryBasics_whenMultipleDifferentQueries_thenSuccessful() {
countOfQueries++;
}
assertEquals(2, countOfQueries);
assertEquals(10, query3.getQueryContext().getPaginationDepth());
assertEquals(10, query3.getQueryContext().getPaginationDepth().intValue());
}

@SneakyThrows
Expand Down
Loading