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: Return errors from typeJoinOne #1716

Merged

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #1711

Description

Returns errors from typeJoinOne instead of ignoring them.

I assume this is a historical accident.

@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 Jul 26, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.6 milestone Jul 26, 2023
@AndrewSisley AndrewSisley requested a review from a team July 26, 2023 16:52
@AndrewSisley AndrewSisley self-assigned this Jul 26, 2023
}

// We have to reset the scan node after appending the new key-filter
if err := n.subType.Init(); err != nil {
log.ErrorE(n.p.ctx, "Sub-type initialization error at scan node reset", err)
return doc
return doc, errors.Wrap("sub-type initialization error at scan node reset", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: Add to errors.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorted

@@ -408,8 +413,7 @@ func (n *typeJoinOne) valuesPrimary(doc core.Doc) core.Doc {

// re-initialize the sub type plan
if err := n.subType.Init(); err != nil {
log.ErrorE(n.p.ctx, "Sub-type initialization error at scan node reset", err)
return doc
return doc, errors.Wrap("sub-type initialization error at scan node reset", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: After adding the above to errors.go, you can use the defined error function here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorted

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

Once the error definition is moved to the errors.go file, all clear to merge from my point of view.

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Patch coverage: 50.00% and project coverage change: +0.01% 🎉

Comparison is base (ca1cb68) 75.37% compared to head (9ecaf95) 75.38%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1716      +/-   ##
===========================================
+ Coverage    75.37%   75.38%   +0.01%     
===========================================
  Files          208      208              
  Lines        21717    21719       +2     
===========================================
+ Hits         16369    16372       +3     
+ Misses        4207     4206       -1     
  Partials      1141     1141              
Flag Coverage Δ
all-tests 75.38% <50.00%> (+0.01%) ⬆️

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

Files Changed Coverage Δ
planner/errors.go 0.00% <0.00%> (ø)
planner/planner.go 76.70% <ø> (ø)
planner/type_join.go 75.19% <56.25%> (ø)

... and 2 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 ca1cb68...9ecaf95. Read the comment docs.

@AndrewSisley AndrewSisley merged commit 5b2182f into sourcenetwork:develop Jul 26, 2023
11 checks passed
@AndrewSisley AndrewSisley deleted the 1711-handle-one-join-errors branch July 26, 2023 17:19
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
## Relevant issue(s)

Resolves sourcenetwork#1711

## Description

Returns errors from typeJoinOne instead of ignoring them.
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.

typeJoinOne silently discards errors in Next
2 participants