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

fix: Handle multiple child index joins #2867

Merged

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #2862

Description

Handles multiple child index joins.

Init is called multiple times (IIRC when joining in this direction it is called 1+NPrimaryDocs times) and the state was preserved between calls. The new resetState func duplicates the overwriting of props set in init, but I see that as worth it (very cheap, and means devs don't have to care what is set outside of the constructor).

@AndrewSisley AndrewSisley added bug Something isn't working area/query Related to the query component labels Jul 24, 2024
@AndrewSisley AndrewSisley added this to the DefraDB v0.13 milestone Jul 24, 2024
@AndrewSisley AndrewSisley requested a review from a team July 24, 2024 21:10
@AndrewSisley AndrewSisley self-assigned this Jul 24, 2024
Copy link
Contributor

@islamaliev islamaliev left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks Andy for the quick fix

f.indexedFields = nil
f.docFields = nil
f.indexIter = nil
f.execInfo = ExecInfo{}
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: f.execInfo.Reset()

Copy link
Contributor Author

@AndrewSisley AndrewSisley Jul 24, 2024

Choose a reason for hiding this comment

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

Oh nice, thanks Islam :)

  • f.execInfo.Reset()

At the moment it panics when executing the request (same error as in ticket).
Note, DocIndexes are not supported in inner request results, hence we have to tolerate a few new docIDs in the short term.
@AndrewSisley AndrewSisley force-pushed the 2862-sec-index-rel-panic branch from 5745d53 to 76a9e0d Compare July 24, 2024 21:29
{
"devices": []map[string]any{
{
"owner_id": "bae-1ef746f8-821e-586f-99b2-4cb1fb9b782f",
Copy link
Contributor

Choose a reason for hiding this comment

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

note: if you wait for #2871 to merge you will be able to use testUtils.NewDocIndex for these too.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Jul 24, 2024

Choose a reason for hiding this comment

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

Yeah, I might do that, then you can merge before you log off - the CI runners seem to be in short supply atm though :(

If you don't merge #2871 within an hour I'll assume you or the CI is sleeping and merge this to unblock the partner/David

Copy link
Contributor

Choose a reason for hiding this comment

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

no worries, if you merge first, I can change your asserts in my PR

Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.44%. Comparing base (5cd0185) to head (76a9e0d).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2867      +/-   ##
===========================================
+ Coverage    79.39%   79.44%   +0.05%     
===========================================
  Files          323      323              
  Lines        24681    24692      +11     
===========================================
+ Hits         19595    19615      +20     
+ Misses        3681     3677       -4     
+ Partials      1405     1400       -5     
Flag Coverage Δ
all-tests 79.44% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
internal/db/fetcher/indexer.go 82.88% <100.00%> (+1.88%) ⬆️

... and 16 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cd0185...76a9e0d. Read the comment docs.

@AndrewSisley AndrewSisley merged commit cca14ac into sourcenetwork:develop Jul 24, 2024
41 of 42 checks passed
@AndrewSisley AndrewSisley deleted the 2862-sec-index-rel-panic branch July 24, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/query Related to the query component bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secondary index on relation panics when querying
2 participants