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

feat: Allow filtering by nil #789

Merged
merged 3 commits into from
Sep 14, 2022
Merged

feat: Allow filtering by nil #789

merged 3 commits into from
Sep 14, 2022

Conversation

AndrewSisley
Copy link
Contributor

@AndrewSisley AndrewSisley commented Sep 12, 2022 •
edited
Loading

Relevant issue(s)

Resolves #410

Description

Allows filtering by nil. Requires sourcenetwork/graphql-go#6 and mod.go should be updated once that is merged.

Todo:

  • Update mod.go once gql-go fork updated

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

  • Debian Linux

@AndrewSisley AndrewSisley added feature New feature or request area/query Related to the query component action/no-benchmark Skips the action that runs the benchmark. labels Sep 12, 2022
@AndrewSisley AndrewSisley added this to the DefraDB v0.3.1 milestone Sep 12, 2022
@AndrewSisley AndrewSisley requested a review from a team September 12, 2022 22:01
@AndrewSisley AndrewSisley self-assigned this Sep 12, 2022
@AndrewSisley AndrewSisley force-pushed the sisley/feat/I410-filter-by-nil branch from ea877e6 to 3a2c338 Compare September 12, 2022 22:01
@AndrewSisley AndrewSisley mentioned this pull request Sep 12, 2022
Closed
@AndrewSisley AndrewSisley force-pushed the sisley/feat/I410-filter-by-nil branch from 3a2c338 to 6112b65 Compare September 13, 2022 19:16
@codecov
Copy link

codecov bot commented Sep 13, 2022 •
edited
Loading

Codecov Report

Merging #789 (9523aac) into develop (e1b663e) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #789      +/-   ##
===========================================
+ Coverage    59.13%   59.17%   +0.04%     
===========================================
  Files          153      153              
  Lines        16977    16994      +17     
===========================================
+ Hits         10039    10057      +18     
+ Misses        6021     6020       -1     
  Partials       917      917
Impacted Files Coverage Δ
connor/ge.go 72.72% <100.00%> (+6.06%) ⬆️
connor/gt.go 71.42% <100.00%> (+4.76%) ⬆️
connor/le.go 72.72% <100.00%> (+6.06%) ⬆️
connor/lt.go 77.27% <100.00%> (+5.05%) ⬆️
query/graphql/parser/filter.go 62.71% <100.00%> (+0.64%) ⬆️
connor/numbers/equality.go 52.17% <0.00%> (+4.34%) ⬆️

@AndrewSisley AndrewSisley force-pushed the sisley/feat/I410-filter-by-nil branch from 6112b65 to 88f0141 Compare September 13, 2022 19:32
fredcarle
fredcarle approved these changes Sep 14, 2022
View reviewed changes
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.

LGTM!

tests/integration/query/simple/with_filter/with_eq_float_test.go
},
Results: []map[string]interface{}{
{
"Name": "Fred",
Copy link
Collaborator

Choose a reason for hiding this comment

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

👌

connor/le.go
@@ -9,6 +9,11 @@ import (
// le does value comparisons to determine whether one
// value is strictly less than another.
func le(condition, data interface{}) (bool, error) {
if condition == nil {
// Only nil is less than or equal to nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

praise: this context comment are appreciated.

shahzadlone
shahzadlone approved these changes Sep 14, 2022
View reviewed changes
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

LGTM!

question(non-blocking): Wondering if the behavior would work for more complicated types as well? if so a few tests for that would be nice, for example:

query {
  users(groupBy: [Age]) {
    Age
    _count(_group: {filter: {Age: {_gt: null}}})
  }
}

@AndrewSisley
Copy link
Contributor Author

LGTM!

question(non-blocking): Wondering if the behavior would work for more complicated types as well? if so a few tests for that would be nice, for example:

query {
  users(groupBy: [Age]) {
    Age
    _count(_group: {filter: {Age: {_gt: null}}})
  }
}

I cant see how it could break (in different ways) there - aggregates run the same filter code as everywhere else.

@AndrewSisley AndrewSisley force-pushed the sisley/feat/I410-filter-by-nil branch from 88f0141 to f2e5907 Compare September 14, 2022 14:16
@AndrewSisley AndrewSisley force-pushed the sisley/feat/I410-filter-by-nil branch from f2e5907 to 9523aac Compare September 14, 2022 14:23
@AndrewSisley AndrewSisley merged commit aa6c7da into develop Sep 14, 2022
@AndrewSisley AndrewSisley deleted the sisley/feat/I410-filter-by-nil branch September 14, 2022 14:38
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@AndrewSisley
feat: Allow filtering by nil (sourcenetwork#789)
b9eadc0 
* Update gql-go version to latest

Includes NullValue support

* Go mod tidy

* Add support for filter by nil
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shahzadlone shahzadlone shahzadlone approved these changes

@fredcarle fredcarle fredcarle approved these changes

Assignees

@AndrewSisley AndrewSisley

Labels
action/no-benchmark Skips the action that runs the benchmark. area/query Related to the query component feature New feature or request
Projects
None yet
Milestone
DefraDB v0.3.1
Development

Successfully merging this pull request may close these issues.

Not way to filter by nil
3 participants
@AndrewSisley @fredcarle @shahzadlone