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

search: simplify case statement #37658

Merged
merged 1 commit into from
Jun 24, 2022
Merged

Conversation

rvantonder
Copy link
Contributor

stacked on #37657

Test plan

semantics-preserving

@cla-bot cla-bot bot added the cla-signed label Jun 24, 2022
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Jun 24, 2022

Codenotify: Notifying subscribers in CODENOTIFY files for diff 4f2c999...683eb80.

Notify File(s)
@beyang internal/search/query/query.go
@camdencheek internal/search/query/query.go
@keegancsmith internal/search/query/query.go

case SearchTypeStandard:
processType = succeeds(substituteConcat(space))
case SearchTypeLiteral:
case SearchTypeStandard, SearchTypeLucky, SearchTypeLiteral:
processType = succeeds(substituteConcat(space))
Copy link
Member

Choose a reason for hiding this comment

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

Trying to make sure I undestand this. Please tell me if any of these statements are inaccurate:

  • All of the below are true for SearchTypeStandard
  • We can have literal patterns and /regex patterns/
  • Space-separated literal patterns will be parsed into Concat nodes
  • /regex patterns/ will be parsed as a single pattern node with the Standard label set
  • /regex patterns/ will never exist in a concat node
  • We can safely join concat nodes with spaces because /regex patterns/ will not exist in a concat node

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, so the end state behavior isn't implemented yet, I'm just organizing this. I need to implement a different Concat processor for standard. Concat nodes always contain patterns (they can be regexp or literal, doesn't matter).

Currently, literal patterns are concatted with space. regexp patterns are concatted with .*. Now, with standard, it's possible to have literal and regexp patterns alternate in the input string for the first time, so the Concat behavior will be different (we don't want to always use space, but right now it does, and consequently, it just means that the regexp pattern is reduced to a literal string). Instead, the end state will work like this:

  • Any sequence of regexp patterns like /foo/ /bar/ /baz/ will be AND-ed
  • Alternation of regexp and literal patterns like /foo/ bar /baz/ will be ANDed
  • Any sequence of literal patterns like foo bar baz will be concatenated with space (preserving literal search behavior as before).

Essentially, everything will be ANDed except for contiguous literal patterns, which will be space, as before. My intuition is that the AND meaning is more intuitive outside of the literal pattern behavior, because unlike literal patterns, regex patterns are explicitly delineated with /.../. For example, it would be a little silly to say /foo/ /bar/ if you prefer to concat those patterns--you would probably (should) prefer to just write /foo bar/.

Combining lucky search with other alternatives (and also the AND interpretation of literal patterns) will cover the entire spectrum of possibilities and hopefully crush ambiguity issues really well.

Does that explanation and plan jives with you?

Copy link
Member

Choose a reason for hiding this comment

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

Does that explanation and plan jives with you?

Yes, this sounds great. Thanks for explaining!

Base automatically changed from rvt/cleanup-json to main June 24, 2022 22:09
@rvantonder rvantonder force-pushed the rvt/lucky-search-uses-standard branch from 9a13bad to 683eb80 Compare June 24, 2022 22:13
@rvantonder rvantonder merged commit e2e5c3b into main Jun 24, 2022
@rvantonder rvantonder deleted the rvt/lucky-search-uses-standard branch June 24, 2022 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants