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: Reset scan node for each join #828

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #827

Description

Resets scan node for each join. Scan node may have completed and will be in a finished state and will return nil for other items.

Specify the platform(s) on which this was tested:

  • Debian Linux

@AndrewSisley AndrewSisley added bug Something isn't working area/query Related to the query component action/no-benchmark Skips the action that runs the benchmark. labels Sep 19, 2022
@AndrewSisley AndrewSisley added this to the DefraDB v0.3.1 milestone Sep 19, 2022
@AndrewSisley AndrewSisley requested a review from a team September 19, 2022 21:41
@AndrewSisley AndrewSisley self-assigned this Sep 19, 2022
Scan node will have completed and will be in a finished state
@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #828 (b40b99a) into develop (bbb9b42) will decrease coverage by 0.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #828      +/-   ##
===========================================
- Coverage    59.59%   59.57%   -0.02%     
===========================================
  Files          155      155              
  Lines        17266    17271       +5     
===========================================
+ Hits         10289    10290       +1     
- Misses        6048     6051       +3     
- Partials       929      930       +1     
Impacted Files Coverage Δ
query/graphql/planner/type_join.go 72.59% <33.33%> (-0.78%) ⬇️

Comment on lines +124 to +132
// bae-fd541c25-229e-5280-b44b-e5c2af3e374d
`{
"name": "Painted House",
"rating": 4.9
}`,
// "bae-d3bc0f38-a2e1-5a26-9cc9-5b3fdb41c6db"
`{
"name": "Go Guide for Rust developers",
"rating": 5.0
Copy link
Member

Choose a reason for hiding this comment

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

question: why not keep the 4 fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are irrelevant, and including them suggests relevance

Copy link
Member

Choose a reason for hiding this comment

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

A little strict xD. It doesn't hinder understanding with them in, however it also doesn't add anything

Copy link
Contributor Author

@AndrewSisley AndrewSisley Sep 19, 2022

Choose a reason for hiding this comment

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

IMO it does, when tracking down the bug the first thing I did (and this was about 50% of the effort) was remove everything extra so I had a minimal failing case. Specificity matters a lot in tests IMO. Really cuts down the possible what-if cases when looking to see why it is failing

Copy link
Member

Choose a reason for hiding this comment

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

fair 👍

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

LGTM

@AndrewSisley AndrewSisley merged commit 7d41679 into develop Sep 19, 2022
@AndrewSisley AndrewSisley deleted the sisley/fix/I827-one-one-bug branch September 19, 2022 21:55
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
Scan node will have completed and will be in a finished state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. 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.

One to one joins not linking properly
3 participants