Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

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
adelinowona merged 10 commits into mongodb:main from adelinowona:csharp5543
May 1, 2025

Conversation

@adelinowona
Copy link
Contributor

@adelinowona adelinowona commented Apr 25, 2025

No description provided.

@adelinowona adelinowona added the feature Adds new user-facing functionality. label Apr 25, 2025
@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?

adelinowona reacted with thumbs up emoji
Copy link
Contributor Author

@adelinowona adelinowona Apr 28, 2025

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

@adelinowona adelinowona Apr 28, 2025

Choose a reason for hiding this comment

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

Should be ok!

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 reacted with thumbs up emoji
/// <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).

adelinowona reacted with thumbs up emoji
_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

@sanych-sun sanych-sun Apr 29, 2025

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

@sanych-sun sanych-sun Apr 29, 2025

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.

adelinowona reacted with thumbs up emoji
/// 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

@sanych-sun sanych-sun Apr 29, 2025

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

@sanych-sun sanych-sun Apr 29, 2025

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

@adelinowona adelinowona Apr 29, 2025

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

@sanych-sun sanych-sun Apr 30, 2025

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

@adelinowona adelinowona Apr 30, 2025

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.

adelinowona reacted with thumbs up emoji
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.

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)

adelinowona reacted with thumbs up emoji
{ "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

@adelinowona adelinowona Apr 30, 2025

Choose a reason for hiding this comment

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

Done, thanks!

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

Reviewers

@BorisDog BorisDog BorisDog approved these changes

@sanych-sun sanych-sun sanych-sun approved these changes

Assignees

No one assigned

Labels

feature Adds new user-facing functionality.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /