Skip to content

CSHARP-5543: Add new options for Atlas Search Text and Phrase operators #1678

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

Merged
merged 10 commits into from
May 1, 2025

Conversation

adelinowona
Copy link
Contributor

No description provided.

@adelinowona adelinowona requested a review from BorisDog April 25, 2025 22:34
@adelinowona adelinowona requested a review from a team as a code owner April 25, 2025 22:34
@adelinowona adelinowona removed the request for review from a team April 28, 2025 15:38
Copy link
Contributor

@BorisDog BorisDog left a comment

Choose a reason for hiding this comment

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

Few minor comments

/// <summary>
/// The score modifier.
/// </summary>
public SearchScoreDefinition<TDocument> Score { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Alphabetic order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// The criteria to use to match the terms in the query. Value can be either "any" or "all".
/// Defaults to "all" if omitted.
/// </summary>
public string MatchCriteria { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check if this can be enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be ok!

@@ -457,6 +470,7 @@ internal sealed class TextSearchDefinition<TDocument> : OperatorSearchDefinition
private readonly SearchFuzzyOptions _fuzzy;
private readonly SearchQueryDefinition _query;
private readonly string _synonyms;
private readonly string _matchCriteria;
Copy link
Contributor

Choose a reason for hiding this comment

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

Alphabetical order.

@adelinowona adelinowona requested a review from BorisDog April 28, 2025 22:17
/// <summary>
/// Represents the criteria used to match terms in a query for the Atlas Search Text operator.
/// </summary>
public enum MatchCriteria
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 should in Search folder (and namespace).

@@ -329,11 +330,23 @@ public PhraseSearchDefinition(
_slop = slop;
}

public PhraseSearchDefinition(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need two ctors? This class is internal so it's safe to change.

/// The criteria to use to match the terms in the query. Value can be either "any" or "all".
/// Defaults to "all" if omitted.
/// </summary>
public MatchCriteria MatchCriteria { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nullable?

Copy link
Member

Choose a reason for hiding this comment

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

I would say we do not need the property be nullable. It should simply have the default value of MatchCriteria.All. Why do we need additional third state of null?

Copy link
Member

Choose a reason for hiding this comment

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

As per discussion with Boris, we probably should have it nullable so the user could explicitly have a 3rd unset state.

/// The criteria to use to match the terms in the query. Value can be either "any" or "all".
/// Defaults to "all" if omitted.
/// </summary>
public MatchCriteria MatchCriteria { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

I would say we do not need the property be nullable. It should simply have the default value of MatchCriteria.All. Why do we need additional third state of null?

/// <summary>
/// The name of the synonym mapping definition in the index definition. Value can't be an empty string.
/// </summary>
public string Synonyms { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

xml-docs says: Value can't be an empty string. Should we enforce this 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.

Similar to my other comment I prefer letting the server error rather than enforcing the condition in the driver. Yes, it's simple but I don't think the driver should be doing that much hand holding.

Copy link
Member

Choose a reason for hiding this comment

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

If we letting server to decide, why do we have it in XML docs then? =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in the XML docs because I mirrored the official docs and it's mentioned there. I didn't think much of it.

Copy link
Contributor

@BorisDog BorisDog left a comment

Choose a reason for hiding this comment

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

Minor changes requested.

SearchPathDefinition<TDocument> path,
SearchQueryDefinition query,
SearchPhraseOptions<TDocument> options) =>
new PhraseSearchDefinition<TDocument>(path, query, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation here and other places.

public int? Slop { get; set; }

/// <summary>
/// The name of the synonym mapping definition in the index definition. Value can't be an empty string.
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: 'Value can't be an empty string': it might sound like null is also an invalid value. I think it's worth clarifying this.

@adelinowona adelinowona requested a review from BorisDog April 30, 2025 16:54
SearchScoreDefinition<TDocument> score)
: base(OperatorType.Phrase, path, score)
SearchPhraseOptions<TDocument> options)
: base(OperatorType.Phrase, path, options?.Score)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also indentation for this line (and in TextSearchDefinition ctor)

}

private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args) =>
new()
{
{ "query", _query.Render() },
{ "fuzzy", () => _fuzzy.Render(), _fuzzy != null },
{ "synonyms", _synonyms, _synonyms != null }
{ "synonyms", _synonyms, _synonyms != null },
{ "matchCriteria", _matchCriteria == MatchCriteria.Any ? "any" : "all", _matchCriteria != null }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to make this more errorproof and check for unexpected values:

MatchCriteriaToString(MatchCriteria? matchCriteria) => matchCriteria switch 
{
  _ => throw...
}

Because if a user sets MatchCriteria=(MatchCriteria)3 we don't want to be sending "all" in such case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

@adelinowona adelinowona requested a review from BorisDog April 30, 2025 20:13
Copy link
Contributor

@BorisDog BorisDog left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@sanych-sun sanych-sun left a comment

Choose a reason for hiding this comment

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

LGTM

@adelinowona adelinowona merged commit d6840c3 into mongodb:main May 1, 2025
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants