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 support for partial matching in the wildcard query #27

Closed
wants to merge 5 commits into from

Conversation

LinZhihao-723
Copy link
Member

Description

In most cases, users want to search for any result containing the given wildcard search string. This is implemented as "partial matching", which is the default search query in clg. This PR implements partial matching as the default option when building the wildcard query.
In terms of the low level implementation details, a partial match wildcard query "${WILDCARD_STRING}" is equivalent to the full match wildcard query *${WILDCARD_STRING}*.

Validation performed

  1. Update the unit tests to cover the partial match.
  2. All linters passed.

}
}

[[nodiscard]] auto get_uncleaned_wildcard_query() const -> std::string const& {
Copy link
Member

Choose a reason for hiding this comment

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

Lets rename get_uncleaned_wildcard_query. Maybe something like get_original_query_string or get_user_query_string. I don't think we need to explicitly call it a wildcard query considering this is a method of the WildcardQuery class (so pretty redundant). I'm a bit indifferent about adding _string as a suffix, but it would probably be necessary if there is ever a Query class in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. I will keep the suffix _string to differentiate it from the actual query and the input.

@junhaoliao
Copy link
Member

As we discussed offline, without partial matching, the current .search() behaviour has caused confusion among users. Shall we revive this PR?

@LinZhihao-723
Copy link
Member Author

As we discussed offline, without partial matching, the current .search() behaviour has caused confusion among users. Shall we revive this PR?

@kirkrodrigues I think we have a clear use case where users want the wildcard to be automatically surrounded by *

@kirkrodrigues
Copy link
Member

As we discussed offline, without partial matching, the current .search() behaviour has caused confusion among users. Shall we revive this PR?

@kirkrodrigues I think we have a clear use case where users want the wildcard to be automatically surrounded by *

Things are a little awkward because users are already using this library in production and the recommended practice is not to change APIs without a major version bump; to support new functionality before the major version bump, the recommendation is to mark the current API for deprecation and then add new APIs with the new functionality.

In light of that, my proposal would be:

  • Add two new classes---SubstringWildcardQuery and FullStringWildcardQuery---that are derived from WildcardQuery.
    • I prefer this compared to a flag since it makes the scope of the query clear.
    • Internally, SubstringWildcardQuery can call the WildcardQuery constructor with the added prefix & suffix wildcards. Eventually, if we need to move the prefix/suffix logic lower in the call hierarchy (e.g., for debugging as you suggested initially), then having a separate class makes this simpler.
  • Mark WildcardQuery for deprecation. In the future, we can make it abstract like @davidlion suggested when we discussed this initially.
  • Mark QueryBuilder's add_wildcard_query for deprecation and add a new version of add_wildcard_query (using Python's syntactic sugar for method overloading) which takes a WildcardQuery instead.
    • One advantage of this new add_wildcard_query is we don't have to keep changing the API of add_wildcard_query whenever the WildcardQuery constructors change. It's also more consistent with add_wildcard_queries.

@LinZhihao-723
Copy link
Member Author

The equivalence has been implemented in PR #62

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.

4 participants