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

Jump to bottom

Structural search insight always shows 0 for most recent data point #50506

Closed
nihaals opened this issue Apr 10, 2023 · 3 comments
Closed

Structural search insight always shows 0 for most recent data point #50506

nihaals opened this issue Apr 10, 2023 · 3 comments
Assignees
@gl-srgr
Labels
bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior.

Comments

@nihaals
Copy link

nihaals commented Apr 10, 2023 •
edited

  • Sourcegraph version: v5.0.0

Steps to reproduce:

  1. Create a "Track changes" insight
  2. Enter a structural search e.g. lang:Python patternType:structural requests.get(...)
  3. Notice the number of results always drops to 0 in the most recent data point in both the preview and the actual insight

Expected behavior:

It shows the correct number of results.

Actual behavior:

It always shows 0 as the number of results.

For example:

image
@chwarwick chwarwick added the bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior. label Apr 12, 2023
@gl-srgr gl-srgr self-assigned this Apr 14, 2023
@gl-srgr
Copy link
Contributor

gl-srgr commented Apr 20, 2023

The issue is that we only pass RepoRevs to the structural search method but if a revision is not empty string then we end up using that revision as the branch name instead of using HEAD, see here.

Zoekt let us know before this logic when we called PartitionRepos that we should be using HEAD, but this information is stored in the IndexedRepoRevs.BranchRepos member which we don't pass along to runStructuralSearch.

I have a few solutions in mind and will run them by @camdencheek

@gl-srgr gl-srgr mentioned this issue Apr 25, 2023
Merged
gl-srgr added a commit that referenced this issue May 3, 2023
@gl-srgr
Structural Search: Modify revisions values to match on Zoekt branches (…
633c37b 
…#51076)

Issue: #50506

The issue manifests for a structural search for any revisions specified
in the query that have both of the following:
1) the revision is a commit and is the latest commit of an indexed
branch in Zoekt
2) the revision is not the branch name indexed in Zoekt (i.e. it is not
a revision specified in `experimentalFeatures.search.index.branches`)

The following fix works for all cases where the indexed branch to query
is `HEAD`. Therefore this will resolve the code insights use case since
code insights repo filter does not accept revisions.

The fix in this PR is intended to be something we can ship quickly and
therefore I hoped to avoid modifying existing logic as much as possible.
For this reason we keep the `SearchJob` arguments and logic unchanged
but just modify the repo revisions passed in.

Note that there is still a remaining issue where if we specify two
revisions for our query and both of those revisions match both criteria
described above, then we do not have enough information to determine
which branch to use since we'll have two branches and neither of them
have a map to their latest commit. To resolve this I believe we'd have
to modify
[`PartitionRepos()`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/search/zoekt/indexed_search.go?L177:6)
and/or
[`IndexedRepoRevs`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/search/zoekt/indexed_search.go?L38)
which would be a bit more involved so I am planning to spin off to a
separate ticket.

## Test plan

<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->

New method has test cases added
@github-actions github-actions bot mentioned this issue May 3, 2023
Merged
github-actions bot pushed a commit that referenced this issue May 3, 2023
@gl-srgr @github-actions
Structural Search: Modify revisions values to match on Zoekt branches (…
2b00717 
…#51076)

Issue: #50506

The issue manifests for a structural search for any revisions specified
in the query that have both of the following:
1) the revision is a commit and is the latest commit of an indexed
branch in Zoekt
2) the revision is not the branch name indexed in Zoekt (i.e. it is not
a revision specified in `experimentalFeatures.search.index.branches`)

The following fix works for all cases where the indexed branch to query
is `HEAD`. Therefore this will resolve the code insights use case since
code insights repo filter does not accept revisions.

The fix in this PR is intended to be something we can ship quickly and
therefore I hoped to avoid modifying existing logic as much as possible.
For this reason we keep the `SearchJob` arguments and logic unchanged
but just modify the repo revisions passed in.

Note that there is still a remaining issue where if we specify two
revisions for our query and both of those revisions match both criteria
described above, then we do not have enough information to determine
which branch to use since we'll have two branches and neither of them
have a map to their latest commit. To resolve this I believe we'd have
to modify
[`PartitionRepos()`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/search/zoekt/indexed_search.go?L177:6)
and/or
[`IndexedRepoRevs`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/search/zoekt/indexed_search.go?L38)
which would be a bit more involved so I am planning to spin off to a
separate ticket.

## Test plan

<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->

New method has test cases added

(cherry picked from commit 633c37b)
unknwon pushed a commit that referenced this issue May 3, 2023
@github-actions @gl-srgr
[Backport 5.0] Structural Search: Modify revisions values to match on…
6d37df2 
… Zoekt branches (#51419)

Issue: #50506

The issue manifests for a structural search for any revisions specified
in the query that have both of the following:
1) the revision is a commit and is the latest commit of an indexed
branch in Zoekt
2) the revision is not the branch name indexed in Zoekt (i.e. it is not
a revision specified in `experimentalFeatures.search.index.branches`)

The following fix works for all cases where the indexed branch to query
is `HEAD`. Therefore this will resolve the code insights use case since
code insights repo filter does not accept revisions.

The fix in this PR is intended to be something we can ship quickly and
therefore I hoped to avoid modifying existing logic as much as possible.
For this reason we keep the `SearchJob` arguments and logic unchanged
but just modify the repo revisions passed in.

Note that there is still a remaining issue where if we specify two
revisions for our query and both of those revisions match both criteria
described above, then we do not have enough information to determine
which branch to use since we&#39;ll have two branches and neither of
them have a map to their latest commit. To resolve this I believe
we&#39;d have to modify
[`PartitionRepos()`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/search/zoekt/indexed_search.go?L177:6)
and/or
[`IndexedRepoRevs`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/search/zoekt/indexed_search.go?L38)
which would be a bit more involved so I am planning to spin off to a
separate ticket.

## Test plan

&lt;!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
--&gt;

New method has test cases added <br> Backport
633c37b from #51076

Co-authored-by: Gary Lee <105310278+gl-srgr@users.noreply.github.com>
@gl-srgr
Copy link
Contributor

gl-srgr commented May 4, 2023

This fix is available as of 5.0.3

@gl-srgr gl-srgr closed this as completed May 4, 2023
@gl-srgr
Copy link
Contributor

gl-srgr commented May 4, 2023

Insight for testing, prior to fix:

Screenshot 2023-05-03 at 6 38 25 PM

Same insight, reprocessed after version with fix deployed:
image

@gl-srgr gl-srgr mentioned this issue May 4, 2023
Closed
VolhaBakanouskaya pushed a commit to VolhaBakanouskaya/sourcegraph-public that referenced this issue Jun 30, 2023
@gl-srgr @ErikaRS
Structural Search: Modify revisions values to match on Zoekt branches…
fc11dfd 
… (#51076)

Issue: sourcegraph/sourcegraph#50506

The issue manifests for a structural search for any revisions specified
in the query that have both of the following:
1) the revision is a commit and is the latest commit of an indexed
branch in Zoekt
2) the revision is not the branch name indexed in Zoekt (i.e. it is not
a revision specified in `experimentalFeatures.search.index.branches`)

The following fix works for all cases where the indexed branch to query
is `HEAD`. Therefore this will resolve the code insights use case since
code insights repo filter does not accept revisions.

The fix in this PR is intended to be something we can ship quickly and
therefore I hoped to avoid modifying existing logic as much as possible.
For this reason we keep the `SearchJob` arguments and logic unchanged
but just modify the repo revisions passed in.

Note that there is still a remaining issue where if we specify two
revisions for our query and both of those revisions match both criteria
described above, then we do not have enough information to determine
which branch to use since we'll have two branches and neither of them
have a map to their latest commit. To resolve this I believe we'd have
to modify
[`PartitionRepos()`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/search/zoekt/indexed_search.go?L177:6)
and/or
[`IndexedRepoRevs`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/search/zoekt/indexed_search.go?L38)
which would be a bit more involved so I am planning to spin off to a
separate ticket.

## Test plan

<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->

New method has test cases added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gl-srgr gl-srgr

Labels
bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@chwarwick @nihaals @gl-srgr