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: Explain of _group with dockeys filter to be []string #1348

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

shahzadlone
Copy link
Member

@shahzadlone shahzadlone commented Apr 13, 2023

Relevant issue(s)

Description

Fixes the bug in simple explain attribute of group node that was causing "docKeys":(func() []string)0x132ea40.
But we expected it to be "docKeys": []string{"bae-6a4c5bc5-b044-5a03-a868-8260af6f2254"}.
Also add a test for it.

How has this been tested?

Integration test + CI

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

  • Manjaro (WSL2)

@shahzadlone shahzadlone added bug Something isn't working area/testing Related to any test or testing suite action/no-benchmark Skips the action that runs the benchmark. area/planner Related to the planner system labels Apr 13, 2023
@shahzadlone shahzadlone added this to the DefraDB v0.5.1 milestone Apr 13, 2023
@shahzadlone shahzadlone requested a review from a team April 13, 2023 12:43
@shahzadlone shahzadlone self-assigned this Apr 13, 2023
@shahzadlone shahzadlone changed the title fix: Simple explain of inner _group with dockeys filter to be []string fix: Explain of inner _group with dockeys filter to be []string Apr 13, 2023
@shahzadlone shahzadlone changed the title fix: Explain of inner _group with dockeys filter to be []string fix: Explain of _group with dockeys filter to be []string Apr 13, 2023
@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #1348 (ba4d1c1) into develop (9ff3eb6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1348   +/-   ##
========================================
  Coverage    70.67%   70.67%           
========================================
  Files          184      184           
  Lines        17820    17819    -1     
========================================
  Hits         12594    12594           
  Misses        4273     4273           
+ Partials       953      952    -1     
Impacted Files Coverage Δ
planner/group.go 87.31% <100.00%> (+1.39%) ⬆️

... and 5 files with indirect coverage changes

if c.DocKeys.HasValue() {
childExplainGraph["docKeys"] = c.DocKeys.Value
childExplainGraph["docKeys"] = c.DocKeys.Value()
Copy link
Contributor

Choose a reason for hiding this comment

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

lol - is one to to keep an eye out for in the future, good spot 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

if only we had a anyButNotAFunc instead of any haha.

Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

LGTM

@shahzadlone shahzadlone merged commit a5cf397 into develop Apr 13, 2023
13 of 16 checks passed
@shahzadlone shahzadlone deleted the lone/test/explain-inner-group-dockey-child branch April 13, 2023 16:05
shahzadlone added a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…ork#1348)

Fixes the bug in simple explain attribute of group node that was causing `"docKeys":(func() []string)0x132ea40`.
But we expected it to be `"docKeys": []string{"bae-6a4c5bc5-b044-5a03-a868-8260af6f2254"}`.
Also add a test for it.
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/planner Related to the planner system area/testing Related to any test or testing suite bug Something isn't working
Projects
None yet
2 participants