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: Add support for lwwr scalar arrays (full replace on update) #115

Merged
merged 3 commits into from
Jan 23, 2022
Merged

feat: Add support for lwwr scalar arrays (full replace on update) #115

merged 3 commits into from
Jan 23, 2022

Conversation

AndrewSisley
Copy link
Contributor

@AndrewSisley AndrewSisley commented Jan 13, 2022 •
edited
Loading

Closes #114

Add support for last-writer-wins scalar arrays of the following types:

  • Boolean
  • Integer
  • Float
  • String

Field may currently only be updated by replacing full set, support for updates to specific indexes, or resizing will be added in other tickets.

To do:

  • Document post merge
  • If merged after sum aggregate, make sure tests are added here
  • Create ticket for validateFieldSchema rework

@AndrewSisley AndrewSisley added feature New feature or request area/query Related to the query component area/schema Related to the schema system labels Jan 13, 2022
@AndrewSisley AndrewSisley self-assigned this Jan 13, 2022
@AndrewSisley AndrewSisley force-pushed the sisley/feat/I114-lwwr-scalar-array branch 2 times, most recently from 316b30f to 53fd7a1 Compare January 13, 2022 18:41
This was referenced Jan 13, 2022
Closed
Merged
jsimnz
jsimnz requested changes Jan 19, 2022
View reviewed changes
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.

Nice solid implementation to get scalar arrays up and going.

Some minor issues on the type conversion stuff, and a general concern for the "smell of the code" for that one validateFieldSchema stuff, but we can prob Look at that section in a different PR and come up with a better long term approach

db/collection_update.go Show resolved Hide resolved
document/document.go Outdated Show resolved Hide resolved
document/encoded.go Show resolved Hide resolved
document/encoded.go Outdated Show resolved Hide resolved
document/encoded.go Outdated Show resolved Hide resolved
@AndrewSisley AndrewSisley force-pushed the sisley/feat/I114-lwwr-scalar-array branch from 53fd7a1 to 7e194c9 Compare January 19, 2022 13:48
@codecov
Copy link

codecov bot commented Jan 19, 2022 •
edited
Loading

Codecov Report

Merging #115 (2e5975c) into develop (8de4435) will decrease coverage by 1.55%.
The diff coverage is 0.87%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #115      +/-   ##
===========================================
- Coverage    54.03%   52.47%   -1.56%     
===========================================
  Files           35       35              
  Lines         3633     3741     +108     
===========================================
  Hits          1963     1963              
- Misses        1441     1547     +106     
- Partials       229      231       +2
Impacted Files Coverage Δ
db/collection_update.go 18.34% <0.00%> (-2.76%) ⬇️
document/encoded.go 0.00% <0.00%> (ø)
query/graphql/schema/descriptions.go 76.14% <0.00%> (-7.70%) ⬇️
document/document.go 43.69% <25.00%> (-0.19%) ⬇️
query/graphql/schema/relations.go 54.63% <0.00%> (ø)

@AndrewSisley AndrewSisley force-pushed the sisley/feat/I114-lwwr-scalar-array branch from 7e194c9 to 1d77d91 Compare January 19, 2022 14:05
@AndrewSisley
Copy link
Contributor Author

Nice solid implementation to get scalar arrays up and going.

Some minor issues on the type conversion stuff, and a general concern for the "smell of the code" for that one validateFieldSchema stuff, but we can prob Look at that section in a different PR and come up with a better long term approach

validateFieldSchema issue: #124

@AndrewSisley AndrewSisley force-pushed the sisley/feat/I114-lwwr-scalar-array branch from 1d77d91 to 7f8abd4 Compare January 19, 2022 14:49
@AndrewSisley AndrewSisley force-pushed the sisley/feat/I114-lwwr-scalar-array branch from 7f8abd4 to 2e5975c Compare January 19, 2022 15:51
jsimnz
jsimnz requested changes Jan 23, 2022
View reviewed changes
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.

All the preview changes look good. Added another comment to investigate, but other than that, all good.

Ping me once that last bit is checked on and ill give the finally approval!

document/document.go
@@ -297,6 +297,10 @@ func (doc *Document) setObject(t core.CType, field string, val *Document) error
}

func (doc *Document) setAndParseType(field string, value interface{}) error {
if value == nil {
return nil
Copy link
Member

Choose a reason for hiding this comment

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

I think on further review it might actually need to be a nil value set on the doc.setCBOR field. Can you investigate further?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so on read of your original comment, but I tested without the setCBOR field (int. test covers this) and all was good.

jsimnz
jsimnz approved these changes Jan 23, 2022
View reviewed changes
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.

Everything resolved! LGTM

@AndrewSisley AndrewSisley merged commit 99bdbdd into develop Jan 23, 2022
@AndrewSisley AndrewSisley deleted the sisley/feat/I114-lwwr-scalar-array branch January 23, 2022 20:39
@orpheuslummis orpheuslummis mentioned this pull request Jan 29, 2022
Closed
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@AndrewSisley
feat: Add support for lwwr scalar arrays (full replace on update) (so…
04b175b 
…urcenetwork#115)

* Handle explicit nil values

* Remove unwanted print lines

* Add support for inline scalar arrays
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsimnz jsimnz jsimnz approved these changes

@shahzadlone shahzadlone Awaiting requested review from shahzadlone

Assignees

@AndrewSisley AndrewSisley

Labels
area/query Related to the query component area/schema Related to the schema system feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement simple lwwr scalar array functionality
2 participants
@AndrewSisley @jsimnz