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

Add phrase operator for Atlas Search #1586

Merged
merged 4 commits into from
Jan 14, 2025
Merged

Add phrase operator for Atlas Search #1586

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23e428f
Add phrase
joykim1005 Dec 23, 2024
911142a
update
joykim1005 Jan 13, 2025
3f581b7
fix
joykim1005 Jan 13, 2025
313c221
fix
joykim1005 Jan 13, 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
58 changes: 58 additions & 0 deletions driver-core/src/main/com/mongodb/client/model/search/PhraseSearchOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright 2008-present MongoDB, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.mongodb.client.model.search;

import com.mongodb.annotations.Beta;
import com.mongodb.annotations.Reason;
import com.mongodb.annotations.Sealed;

/**
* @see SearchOperator#phrase(SearchPath, String)
* @see SearchOperator#phrase(Iterable, Iterable)
* @since 5.3
*/

@Sealed
@Beta(Reason.CLIENT)
public interface PhraseSearchOperator extends SearchOperator {
@Override
PhraseSearchOperator score(SearchScore modifier);

/**
* Creates a new {@link PhraseSearchOperator} that uses slop.
*
* @return A new {@link PhraseSearchOperator}.
*/
PhraseSearchOperator slop();

/**
* Creates a new {@link PhraseSearchOperator} that uses slop. The default value is 0.
*
* @param slop The allowable distance between words in the query phrase.
* @return A new {@link PhraseSearchOperator}.
*/
PhraseSearchOperator slop(int slop);

/**
* Creates a new {@link PhraseSearchOperator} that uses synonyms.
*
* @param name The name of the synonym mapping.
* @return A new {@link PhraseSearchOperator}.
*
* @mongodb.atlas.manual atlas-search/synonyms/ Synonym mappings
*/
PhraseSearchOperator synonyms(String name);
}
14 changes: 12 additions & 2 deletions driver-core/src/main/com/mongodb/client/model/search/SearchConstructibleBsonElement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
final class SearchConstructibleBsonElement extends AbstractConstructibleBsonElement<SearchConstructibleBsonElement> implements
MustCompoundSearchOperator, MustNotCompoundSearchOperator, ShouldCompoundSearchOperator, FilterCompoundSearchOperator,
ExistsSearchOperator, TextSearchOperator, AutocompleteSearchOperator,
NumberNearSearchOperator, DateNearSearchOperator, GeoNearSearchOperator,
NumberNearSearchOperator, DateNearSearchOperator, GeoNearSearchOperator, PhraseSearchOperator,
ValueBoostSearchScore, PathBoostSearchScore, ConstantSearchScore, FunctionSearchScore,
GaussSearchScoreExpression, PathSearchScoreExpression,
FacetSearchCollector,
Expand Down Expand Up @@ -81,13 +81,23 @@ public SearchConstructibleBsonElement fuzzy(final FuzzySearchOptions options) {
}

@Override
public TextSearchOperator synonyms(final String name) {
public SearchConstructibleBsonElement synonyms(final String name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Below, the implementation is:

        return newWithMutatedValue(doc -> {
            doc.remove("fuzzy");
            doc.append("synonyms", notNull("name", name));
        });

Previously, this was used just for text, so removing "fuzzy" was mostly harmless. However, now the behaviour is undocumented in PhraseSearchOperator synonyms, and no longer makes sense.

For example, I don't see in principle why synonyms can't be applied before fuzzy matching, and the server might (hypothetically) add this in the future to either text or phrase search. When that happens, the API becomes broken, and the only way out is to break backwards compatibility by removing the remove. Of course, I expect the chances of this actually happening are low, but this is still a problem with the API, that we should at least avoid in the future (that is, we should never "correct" things, but throw a validation exception, though in such cases, we should let the server provide the exception).

We could consider deprecating the "and does not use X" docs for text synonyms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the explanation. The PhraseSearchOperator does not have the "fuzzy" option, so I originally thought that doc.remove("fuzzy") would not have an impact on the implementation above (since the key "fuzzy" is not present in the map that we are trying to remove from). But it makes sense that if I were to use the same synonyms() the removal of "fuzzy" does not make sense in terms of phrase operator's synonyms().

Q: Are you suggesting that we should get rid of doc.remove("fuzzy") completely and throw a validation exception instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot remove it without breaking backwards compatibility. For example, users might be creating a default options for text that might call fuzzy. Then, they "customize" this default by using the synonyms method. If we stopped removing fuzzy, code that previously worked would result in a server error.

We can either (in my order of preference):

  1. Do nothing
  2. Deprecate
  3. Break backwards compat by removing remove
  4. Create two versions of synonyms, which makes the API messy

@stIncMale Could you give a secondary review to this?

Copy link
Member

@stIncMale stIncMale Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Previously, this was used just for text, so removing "fuzzy" was mostly harmless.

This was not only harmless, but by design:

  • text.fuzzy and text.synonyms are mutually exclusive (and are allowed to be both omitted).
  • We wanted to allow building a TextSearchOperator based on another TextSearchOperator, which means, we had to allow calling TextSearchOperator.synonyms after TextSearchOperator.fuzzy having been called, and vice versa. That means each of these methods must undo the other one1.

I agree with Maxim, we should not piggy-back on SearchConstructibleBsonElement.synonyms when implementing PhraseSearchOperator. But we are in no trouble. We simply need to create another subclass of AbstractConstructibleBsonElement implementing PhraseSearchOperator, and implement synonyms there the way it is suitable for PhraseSearchOperator. You may see that this approach has been used multiple times in the past: there are multiple subclasses of both AbstractConstructibleBsonElement and AbstractConstructibleBson, and I remember that at least once I created a subclass because of the issue similar to the one at hand.

1 While TextSearchOperator.synonyms and TextSearchOperator.fuzzy undo each other, supporting the idea of building a TextSearchOperator based on another TextSearchOperator, there is no way of unsetting both them once one of them is set. That is, I completely failed to implement the API in a way that fully supports the aforementioned idea. Fortunately, fixing it (finishing the implementation) does not require any breaking changes.

Additionally, there is currently no way to build a TextSearchOperator based on another one in a way that changes the required fields. Again, supporting this does not require breaking changes. But the fact that I did not do that, is quite embarrassing.

Copy link
Member

@stIncMale stIncMale Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We talked about this with Maxim in Slack, and I created JAVA-5752 Re-visit some aspects of the Atlas search API design before moving it out of beta
 as a result. Given that the API is in beta, we have a chance to re-visit some of the design decisions, and it is worth doing. I mentioned the ticket here just because the current discussion initiated it, not because I see it relevant to the current PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Valentin's suggestion to create another implementing class looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added another implementing class!

return newWithMutatedValue(doc -> {
doc.remove("fuzzy");
doc.append("synonyms", notNull("name", name));
});
}

@Override
public PhraseSearchOperator slop() {
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 this is not necessary. The slop field, if omitted, defaults to using 0, so users can omit it here, instead of setting it to 0.

Is there a similar method in another part of this API, that this is based on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I was basing it off with the fuzzy() method in the same file. If the user calls the fuzzy() method, it defaults the FuzzySearchOptions to be an EMPTY_IMMUTABLE, so I thought that having an option for the users to call slop() with no parameter could also default the slop to be 0.

I was unaware that we don't have to explicitly set the default value inside of a method. I can go ahead and remove it!

Copy link
Member

@stIncMale stIncMale Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with Maxim that we should not introduce PhraseSearchOperator.slop() (the no-argument overload).

TextSearchOperator.fuzzy() is different enough from PhraseSearchOperator.slop() to give meaning to TextSearchOperator.fuzzy(), despite PhraseSearchOperator.slop() not being meaningful:

  • Similarities: both text.fuzzy and phrase.slope are optional fields.
  • Differences: not specifying text.fuzzy is not equivalent to specifying text.fuzzy with the default value (which is the empty document). Not specifying phrase.slope is equivalent to specifying phrase.slope with the default value (which is 0). This means that the TextSearchOperator for which TextSearchOperator.fuzzy() was called is not equivalent to one for which it wasn't called. However, the PhraseSearchOperator for which PhraseSearchOperator.slop() was called is equivalent to one for which it wasn't called.

Thus, you can see why TextSearchOperator.fuzzy() is needed, while PhraseSearchOperator.slop() is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you Valentin for the explanation! I went ahead and removed slop().

return slop(0);
}

@Override
public PhraseSearchOperator slop(final int slop) {
return newWithAppendedValue("slop", notNull("slop", slop));
}

@Override
public AutocompleteSearchOperator anyTokenOrder() {
return newWithAppendedValue("tokenOrder", "any");
Expand Down
30 changes: 30 additions & 0 deletions driver-core/src/main/com/mongodb/client/model/search/SearchOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,36 @@ static GeoNearSearchOperator near(final Point origin, final Number pivot, final
.append("pivot", notNull("pivot", pivot)));
}

/**
* Returns a {@link SearchOperator} that performs a search for documents containing an ordered sequence of terms.
*
* @param path The field to be searched.
* @param query The string to search for.
* @return The requested {@link SearchOperator}.
* @mongodb.atlas.manual atlas-search/phrase/ phrase operator
*/
static PhraseSearchOperator phrase(final SearchPath path, final String query) {
return phrase(singleton(notNull("path", path)), singleton(notNull("query", query)));
}

/**
* Returns a {@link SearchOperator} that performs a search for documents containing an ordered sequence of terms.
*
* @param paths The non-empty fields to be searched.
* @param queries The non-empty strings to search for.
* @return The requested {@link SearchOperator}.
* @mongodb.atlas.manual atlas-search/phrase/ phrase operator
*/
static PhraseSearchOperator phrase(final Iterable<? extends SearchPath> paths, final Iterable<String> queries) {
Iterator<? extends SearchPath> pathIterator = notNull("paths", paths).iterator();
isTrueArgument("paths must not be empty", pathIterator.hasNext());
Iterator<String> queryIterator = notNull("queries", queries).iterator();
isTrueArgument("queries must not be empty", queryIterator.hasNext());
String firstQuery = queryIterator.next();
return new SearchConstructibleBsonElement("phrase", new Document("path", combineToBsonValue(pathIterator, false))
.append("query", queryIterator.hasNext() ? queries : firstQuery));
}

/**
* Creates a {@link SearchOperator} from a {@link Bson} in situations when there is no builder method that better satisfies your needs.
* This method cannot be used to validate the syntax.
Expand Down
4 changes: 3 additions & 1 deletion .../src/test/functional/com/mongodb/client/model/search/AggregatesSearchIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
import static com.mongodb.client.model.search.SearchOperator.exists;
import static com.mongodb.client.model.search.SearchOperator.near;
import static com.mongodb.client.model.search.SearchOperator.numberRange;
import static com.mongodb.client.model.search.SearchOperator.phrase;
import static com.mongodb.client.model.search.SearchOperator.text;
import static com.mongodb.client.model.search.SearchOptions.searchOptions;
import static com.mongodb.client.model.search.SearchPath.fieldPath;
Expand Down Expand Up @@ -608,7 +609,8 @@ private static Stream<Arguments> searchAndSearchMetaArgs() {
dateRange(fieldPath("fieldName6"))
.lte(Instant.ofEpochMilli(1)),
near(0, 1.5, fieldPath("fieldName7"), fieldPath("fieldName8")),
near(Instant.ofEpochMilli(1), Duration.ofMillis(3), fieldPath("fieldName9"))
near(Instant.ofEpochMilli(1), Duration.ofMillis(3), fieldPath("fieldName9")),
phrase(fieldPath("fieldName10"), "term6")
))
.minimumShouldMatch(1)
.mustNot(singleton(
Expand Down
82 changes: 82 additions & 0 deletions driver-core/src/test/unit/com/mongodb/client/model/search/SearchOperatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,88 @@ void near() {
);
}

@Test
void phrase() {
assertAll(
() -> assertThrows(IllegalArgumentException.class, () ->
// queries must not be empty
SearchOperator.phrase(singleton(fieldPath("fieldName")), emptyList())
),
() -> assertThrows(IllegalArgumentException.class, () ->
// paths must not be empty
SearchOperator.phrase(emptyList(), singleton("term"))
),
() -> assertEquals(
new BsonDocument("phrase",
new BsonDocument("path", fieldPath("fieldName").toBsonValue())
.append("query", new BsonString("term"))
),
SearchOperator.phrase(
fieldPath("fieldName"),
"term")
.toBsonDocument()
),
() -> assertEquals(
new BsonDocument("phrase",
new BsonDocument("path", new BsonArray(asList(
fieldPath("fieldName").toBsonValue(),
wildcardPath("wildc*rd").toBsonValue())))
.append("query", new BsonArray(asList(
new BsonString("term1"),
new BsonString("term2"))))
),
SearchOperator.phrase(
asList(
fieldPath("fieldName"),
wildcardPath("wildc*rd")),
asList(
"term1",
"term2"))
.toBsonDocument()
),
() -> assertEquals(
new BsonDocument("phrase",
new BsonDocument("path", fieldPath("fieldName").toBsonValue())
.append("query", new BsonString("term"))
.append("synonyms", new BsonString("synonymMappingName"))
),
SearchOperator.phrase(
singleton(fieldPath("fieldName")),
singleton("term"))
.synonyms("synonymMappingName")
.toBsonDocument()
),
() -> assertEquals(
new BsonDocument("phrase",
new BsonDocument("path", fieldPath("fieldName").toBsonValue())
.append("query", new BsonString("term"))
.append("synonyms", new BsonString("synonymMappingName"))
.append("slop", new BsonInt32(0))
),
SearchOperator.phrase(
singleton(fieldPath("fieldName")),
singleton("term"))
.synonyms("synonymMappingName")
.slop()
.toBsonDocument()
),
() -> assertEquals(
new BsonDocument("phrase",
new BsonDocument("path", fieldPath("fieldName").toBsonValue())
.append("query", new BsonString("term"))
.append("synonyms", new BsonString("synonymMappingName"))
.append("slop", new BsonInt32(5))
),
SearchOperator.phrase(
singleton(fieldPath("fieldName")),
singleton("term"))
.synonyms("synonymMappingName")
.slop(5)
.toBsonDocument()
)
);
}

private static SearchOperator docExamplePredefined() {
return SearchOperator.exists(
fieldPath("fieldName"));
Expand Down
21 changes: 21 additions & 0 deletions driver-scala/src/main/scala/org/mongodb/scala/model/search/SearchOperator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,27 @@ object SearchOperator {
def near(origin: Point, pivot: Number, paths: Iterable[_ <: FieldSearchPath]): GeoNearSearchOperator =
JSearchOperator.near(origin, pivot, paths.asJava)

/**
* Returns a `SearchOperator` that performs a search for documents containing an ordered sequence of terms.
*
* @param path The field to be searched.
* @param query The string to search for.
* @return The requested `SearchOperator`.
* @see [[https://www.mongodb.com/docs/atlas/atlas-search/phrase/ phrase operator]]
*/
def phrase(path: SearchPath, query: String): PhraseSearchOperator = JSearchOperator.phrase(path, query)

/**
* Returns a `SearchOperator` that performs a search for documents containing an ordered sequence of terms.
*
* @param paths The non-empty fields to be searched.
* @param queries The non-empty strings to search for.
* @return The requested `SearchOperator`.
* @see [[https://www.mongodb.com/docs/atlas/atlas-search/phrase/ phrase operator]]
*/
def phrase(paths: Iterable[_ <: SearchPath], queries: Iterable[String]): PhraseSearchOperator =
JSearchOperator.phrase(paths.asJava, queries.asJava)

/**
* Creates a `SearchOperator` from a `Bson` in situations when there is no builder method that better satisfies your needs.
* This method cannot be used to validate the syntax.
Expand Down
8 changes: 8 additions & 0 deletions driver-scala/src/main/scala/org/mongodb/scala/model/search/package.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,14 @@ package object search {
@Beta(Array(Reason.CLIENT))
type TextSearchOperator = com.mongodb.client.model.search.TextSearchOperator

/**
* @see `SearchOperator.phrase(String, SearchPath)`
* @see `SearchOperator.phrase(Iterable, Iterable)`
*/
@Sealed
@Beta(Array(Reason.CLIENT))
type PhraseSearchOperator = com.mongodb.client.model.search.PhraseSearchOperator
katcharov marked this conversation as resolved.
Show resolved Hide resolved

/**
* @see `SearchOperator.autocomplete(String, FieldSearchPath)`
* @see `SearchOperator.autocomplete(Iterable, FieldSearchPath)`
Expand Down