fix: Move field id off of schema #2336

AndrewSisley · 2024-02-21T17:39:04Z

Relevant issue(s)

Resolves #2334

Description

Moves field id off of schema and onto collection.

It is now a local property and does not need to be the same across multiple nodes. The new Fields property on CollectionDescription will be where other local field stuff is move too from the schema (relation_name, descriptions, secondary relation fields).

This change is a prerequisite of allowing fields to be deleted and schema branching (see #2333 for more info on that if currious)

codecov · 2024-02-21T18:00:57Z

Codecov Report

Attention: Patch coverage is 94.58333% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 74.91%. Comparing base (9534113) to head (1b70a23).

❗ Current head 1b70a23 differs from pull request most recent head c9bb80b. Consider uploading reports for the commit c9bb80b to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2336      +/-   ##
===========================================
+ Coverage    74.73%   74.91%   +0.18%     
===========================================
  Files          256      257       +1     
  Lines        25254    25314      +60     
===========================================
+ Hits         18872    18962      +90     
+ Misses        5093     5066      -27     
+ Partials      1289     1286       -3

Flag	Coverage Δ
all-tests	`74.91% <94.58%> (+0.18%)`	⬆️

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

Files	Coverage Δ
client/document.go	`70.64% <100.00%> (ø)`
core/encoding.go	`64.23% <100.00%> (ø)`
db/base/collection_keys.go	`84.62% <100.00%> (+4.62%)`	⬆️
db/collection_get.go	`72.22% <ø> (ø)`
db/collection_index.go	`90.91% <100.00%> (-0.08%)`	⬇️
db/collection_update.go	`71.28% <100.00%> (ø)`
db/description/schema.go	`61.75% <ø> (-1.21%)`	⬇️
db/errors.go	`61.09% <100.00%> (-1.17%)`	⬇️
db/fetcher/encoded_doc.go	`76.14% <100.00%> (ø)`
db/fetcher/errors.go	`54.55% <ø> (+18.18%)`	⬆️
... and 18 more

... and 5 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 9534113...c9bb80b. Read the comment docs.

fredcarle

A few comments to resolve before I can approve :)

fredcarle · 2024-02-21T21:38:08Z

client/descriptions.go

-	for _, field := range schema.Fields {
-		if field.ID == id {
-			return field, true
+func (def CollectionDefinition) GetFieldByName(fieldName string) (FieldDescription, bool) {


thought: It feels al little weird to have CollectionDefinition methods mixed in here.

Okay, I did like having all the GetFieldByName next to each other, but that's partly because the Descriptions are all jumbled up in this one file anyway, and in a way its odd that Definition isn't.

Will let this discussion sit for a while and you/anyone-else talk some more before I try and make a choice RE its location :)

Wait and see

CollectionDefinition is actually defined in a poor place, at the top of client/collection.go - I'd rather not add a bunch of stuff in there. I think it should be moved to a definitions.go file along with FieldDefinition and the funcs they both host. Please let me know if you want that done in this PR, otherwise they might remain where they are for now.

I moved them all in this PR, in a separate commit. Is nicer and it might not get done if we don't do it now.

fredcarle · 2024-02-23T15:33:13Z

client/descriptions.go

+//
+// It draws it's information from the [CollectionFieldDescription] on the [CollectionDescription],
+// and the [SchemaFieldDescription] on the [SchemaDescription].
+type FieldDescription struct {


suggestion: It really threw me off to see FieldDescription still in existance. It's an exact duplicate of SchemaFieldDescription with tha addition of the ID field. Every function that returns FieldDescription or uses it is a method of CollectionDescription or collection and it's odd that when FieldDescription is returned you have to construct the struct with NewFieldDescription. A much cleaner approach would be to embed the SchemaFieldDescription into the CollectionFieldDescription.

It really threw me off to see FieldDescription still in existance.

That's not good - do you have a rough idea as to how much of that was caused my terrible setting of expectations in discord, vs the actual change?

Every function that returns FieldDescription or uses it is a method of CollectionDescription

Yes, that kind of felt like it was breaking expectations when I was consuming it. It kind of flows better at the moment IMO, but that is very possibly because of the limited number of properties on collection-field.

A much cleaner approach would be to embed the SchemaFieldDescription into the CollectionFieldDescription

I do very strongly disagree with this. It creates a large amount of ambiguity in the data model and is kind of reverting back to the starting point where collection had a pointer to schema on it. Amongst other things, it would be very very confusing when mutating collection field stuff as to what you could and couldn't mutate - it is bad with a pointer, and much worse when embedding.

CollectionFieldDescription does not have a SchemaFieldDescription, and in the possibly quite near future, not all CollectionFieldDescription will even have a corresponding SchemaFieldDescription (think secondary fields in a relation).

I did originally have a todo here about moving this struct to the core package, but decided it was nice having the funcs that return it as public. Maybe this choice should be revisited, along with the fact that the functions returning it are hosted on CollectionFieldDescription (which feels uglier the longer I look at it, even though it is kind of convenient atm).

Discuss further, maybe move stuff

That's not good - do you have a rough idea as to how much of that was caused my terrible setting of expectations in discord, vs the actual change?

I though FieldDescription would become SchemaFieldDescription and CollectionFieldDescription. Not have the two new struct plus FieldDescription.

I do very strongly disagree with this. It creates a large amount of ambiguity in the data model

It feels very ambiguous the way it is now in my opinion.

is kind of reverting back to the starting point where collection had a pointer to schema on it

I disagree. Everywhere FieldDescription is used, both collection and schema description are needed to build the struct. It would be simple to remove the confusion and the have the same functionality you have now with something like this

type CollectionFieldDescription struct { Name string ID FieldID schemaField immutable.Option[SchemaFieldDescription] `json:"-"`

}

I disagree. Everywhere FieldDescription is used, both collection and schema description are needed to build the struct. It would be simple to remove the confusion and the have the same functionality you have now with something like this

Only at the moment, because we have only moved one property.

As far as I can tell it would only get worse with more fields in the CollectionFieldDescription.

I have pushed changes that I am happy with. I realized part way through that (what was) FieldDescription is basically what CollectionDefinition is, but at a field level. So I ended up just cleaning up the GetFieldByName funcs and renaming it.

CollectionDefinition will eventually suffer from the same (and worse) ambiguity that FieldDescription.Name does when embedding, however at the moment we were able to get away with it because there is currently no overlap in properties. This is almost certainly going to change, and CollectionDefinition will need to be changed for that (redeclaration of it's components in a way that eliminates ambiguity over what should be used, for example a single Fields []FieldDefinition property, containing a merged set of collection and schema fields).

and that problem is moderately likely to happen with a few other props in the future

To expand on this, I see the following properties likely to clash, and they may have different values on schema vs collection:

Name (if/when it becomes a label this will be much worse, where it's value may be different on schema vs collection)

Description, I definitely want this on Collection, and there is value on having one on Schema too (overridable by collection

RelationName, I want this on Collection, but we could also allow it to be defined on Schema in the case where users wish to include both halves of a relation on the global schema. In such a case I'm not sure we'd want to allow different values to be set, but we could also allow it to be overrideable like Description and Name.

Potentially ACP field level stuff too, I can imaging people might want to be able to set field level stuff on both the schema, and allow local collections to define additional rules on top of the global ones.

Islam (#2339 (comment)):

todo: I feel like it's wrong having almost identical structs with SchemaFieldDescription.

Moreover, you draw an analogy with collectin/schema description, but these 2 are embedded into CollectionDefinition.
But this one is not. It's inconsistent. I you think that CollectionFieldDescription is too small and has only 1 unique field ID we can make it:

type FieldDefinition struct { schemaField SchemaFieldDescription collectionFieldID int }

And you won't need to copy&past all method below (IsObject, IsObjectArray, ...)

Moreover, you draw an analogy with collectin/schema description, but these 2 are embedded into CollectionDefinition.

They shouldn't be long term, please see my earlier comment:

This is almost certainly going to change, and CollectionDefinition will need to be changed for that (redeclaration of it's components in a way that eliminates ambiguity over what should be used, for example a single Fields []FieldDefinition property, containing a merged set of collection and schema fields).

And you won't need to copy&past all method below (IsObject, IsObjectArray, ...)

I consider copy-paste here to be significantly cheaper than detangling inheritance hierarchies, especially coupled with how embedding structs in Go works (is half flattened, but not quite, so the end interface is not the same as declaring the properties in place). And as mentioned further up, these property sets atm are very similar because only one has been moved, they will diverge more so in the future.

And you won't need to copy&past all method below (IsObject, IsObjectArray, ...)

Discussion point bypassed by Islam's suggestion of moving these functions onto FieldKind.

db/collection.go

planner/mapper/mapper.go

client/descriptions.go

fredcarle · 2024-02-23T17:53:05Z

client/descriptions.go

+// IsObject returns true if this field is an object type.
+func (f SchemaFieldDescription) IsObject() bool {
+	return (f.Kind == FieldKind_FOREIGN_OBJECT) ||
+		(f.Kind == FieldKind_FOREIGN_OBJECT_ARRAY)
+}
+
+// IsObjectArray returns true if this field is an object array type.
+func (f SchemaFieldDescription) IsObjectArray() bool {
+	return (f.Kind == FieldKind_FOREIGN_OBJECT_ARRAY)
+}
+
+// IsRelation returns true if this field is a relation.
+func (f SchemaFieldDescription) IsRelation() bool {
+	return f.RelationName != ""
+}
+
+// IsArray returns true if this field is an array type which includes inline arrays as well
+// as relation arrays.
+func (f SchemaFieldDescription) IsArray() bool {
+	return f.Kind == FieldKind_BOOL_ARRAY ||
+		f.Kind == FieldKind_INT_ARRAY ||
+		f.Kind == FieldKind_FLOAT_ARRAY ||
+		f.Kind == FieldKind_STRING_ARRAY ||
+		f.Kind == FieldKind_FOREIGN_OBJECT_ARRAY ||
+		f.Kind == FieldKind_NILLABLE_BOOL_ARRAY ||
+		f.Kind == FieldKind_NILLABLE_INT_ARRAY ||
+		f.Kind == FieldKind_NILLABLE_FLOAT_ARRAY ||
+		f.Kind == FieldKind_NILLABLE_STRING_ARRAY
+}


thought: Another not ideal side effect of this is that we need to dupliucate these methods.

That is quite cheap, I'm not worried about that.

I don't think you can justify copy&paste by performance characteristics.

I'd say you either make one struct embed the other or create something like client.IsArrayKind(Kind)

cheap in terms of dev cost, performance is not relevant here

client.IsArrayKind(Kind) or Kind.IsArray() is a nice thought though, can do the same for the others too.

Move Kind related funcs

AndrewSisley · 2024-02-27T16:26:22Z

client/definitions.go

+//
+// It is to [CollectionFieldDescription] and [SchemaFieldDescription] what [CollectionDefinition]
+// is to [CollectionDescription] and [SchemaDescription].
+type FieldDefinition struct {


Islam:

suggestion: would be great to have some information on why it exists and how should it be used.
Especially now that we have 3 different field descriptions/definitions

Will add something (and maybe to CollectionDefinition too), thanks Islam :)

expand FieldDefinition docs

AndrewSisley · 2024-02-27T16:32:57Z

client/index.go

-				}
+			if fieldsMap[field.Name] {
+				// If the FieldDescription has already been added to the result do not add it a second time
+				// this can happen if a field if referenced by multiple indexes


Islam (#2339 (comment)):

typo: "if a field referenced"

Will sort out, thanks :)

fix typo

AndrewSisley · 2024-02-27T16:34:57Z

planner/mapper/mapper.go

@@ -267,7 +267,7 @@ func resolveAggregates(
 	inputFields []Requestable,
 	mapping *core.DocumentMapping,
 	collectionName string,
-	schema client.SchemaDescription,
+	schema client.CollectionDefinition,


Islam (#2339 (comment)):

todo: rename the param to colDef or something similar please

My bad, thanks for spotting :)

Rename param

fredcarle

I have to say I'm still struggling with the FieldDefinition but the latest changes to the PR make it overall much better than it was initially. I'll give you the benefit of the doubt as to the future benefits that FieldDefinition will bring.

Approving with a few minor comments to resolve before merge.

client/descriptions.go

islamaliev

Overall looks good. But as with Fred, this FieldDescription makes me a bit uncomfortable.
Approving with the promise that it will get better in the near future.

islamaliev · 2024-02-28T07:28:44Z

client/descriptions.go

+
+// IsArray returns true if this FieldKind is an array type which includes inline arrays as well
+// as relation arrays.
+func (f FieldKind) IsArray() bool {


praise: looks much better now

Test ids have changed and are fixed in a different commit.

AndrewSisley added bug Something isn't working feature New feature or request area/schema Related to the schema system labels Feb 21, 2024

AndrewSisley added this to the DefraDB v0.10 milestone Feb 21, 2024

AndrewSisley requested a review from a team February 21, 2024 17:39

AndrewSisley self-assigned this Feb 21, 2024

AndrewSisley force-pushed the 2334-field-id-mv branch from b75833d to 5010780 Compare February 21, 2024 17:47

AndrewSisley changed the title ~~fix: Move field id off of schema~~ fix: Move field id off of schema Feb 21, 2024

fredcarle reviewed Feb 23, 2024

View reviewed changes

client/descriptions.go Outdated Show resolved Hide resolved

fredcarle reviewed Feb 23, 2024

View reviewed changes

AndrewSisley force-pushed the 2334-field-id-mv branch from 8f741ba to 414671e Compare February 23, 2024 18:37

AndrewSisley requested a review from fredcarle February 23, 2024 18:43

AndrewSisley force-pushed the 2334-field-id-mv branch from 14bb4d9 to 714059e Compare February 23, 2024 19:10

AndrewSisley mentioned this pull request Feb 23, 2024

refactor: Generate field ids using a sequence #2339

Merged

AndrewSisley commented Feb 27, 2024

View reviewed changes

AndrewSisley force-pushed the 2334-field-id-mv branch from 2a2ed85 to de3b5e8 Compare February 27, 2024 16:50

AndrewSisley requested a review from islamaliev February 27, 2024 17:42

fredcarle approved these changes Feb 28, 2024

View reviewed changes

client/descriptions.go Outdated Show resolved Hide resolved

client/descriptions.go Outdated Show resolved Hide resolved

client/descriptions.go Outdated Show resolved Hide resolved

islamaliev approved these changes Feb 28, 2024

View reviewed changes

AndrewSisley force-pushed the 2334-field-id-mv branch from 44c6075 to 1b70a23 Compare February 28, 2024 15:34

AndrewSisley added 6 commits February 28, 2024 10:45

Remove commented out code

022fa09

Remove dead code

7051e94

Move field id onto collection

43d2e37

Test ids have changed and are fixed in a different commit.

FIXUP - Fix all the test IDs

1fd424e

Document breaking changes

8c3fcb2

FIXUP - Clean separation between field def/desc

ddf3c48

AndrewSisley added 6 commits February 28, 2024 10:45

PR FIXUP - Move definitions to new file

1841e7a

PR FIXUP - Expand Definition docs

1104688

PR FIXUP - Fix typo

0f4179b

PR FIXUP - Rename mapper variable

27a9c42

PR FIXUP - FieldKind.IsArray etc

b4dec98

PR FIXUP - Remove extra brackets

c9bb80b

AndrewSisley force-pushed the 2334-field-id-mv branch from 1b70a23 to c9bb80b Compare February 28, 2024 15:46

AndrewSisley merged commit db75564 into sourcenetwork:develop Feb 28, 2024
28 of 29 checks passed

AndrewSisley deleted the 2334-field-id-mv branch February 28, 2024 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Move field id off of schema #2336

fix: Move field id off of schema #2336

AndrewSisley commented Feb 21, 2024

codecov bot commented Feb 21, 2024 •

edited

Loading

fredcarle left a comment

fredcarle Feb 21, 2024

AndrewSisley Feb 23, 2024 •

edited

Loading

AndrewSisley Feb 23, 2024

AndrewSisley Feb 23, 2024

fredcarle Feb 23, 2024

AndrewSisley Feb 23, 2024 •

edited

Loading

fredcarle Feb 23, 2024

AndrewSisley Feb 23, 2024

fredcarle Feb 23, 2024

AndrewSisley Feb 23, 2024

AndrewSisley Feb 23, 2024 •

edited

Loading

AndrewSisley Feb 27, 2024

AndrewSisley Feb 27, 2024 •

edited

Loading

AndrewSisley Feb 27, 2024

fredcarle Feb 23, 2024

AndrewSisley Feb 23, 2024

islamaliev Feb 27, 2024

AndrewSisley Feb 27, 2024

AndrewSisley Feb 27, 2024 •

edited

Loading

AndrewSisley Feb 27, 2024

AndrewSisley Feb 27, 2024 •

edited

Loading

AndrewSisley Feb 27, 2024

AndrewSisley Feb 27, 2024 •

edited

Loading

AndrewSisley Feb 27, 2024

AndrewSisley Feb 27, 2024 •

edited

Loading

fredcarle left a comment

islamaliev left a comment

islamaliev Feb 28, 2024

fix: Move field id off of schema #2336

fix: Move field id off of schema #2336

Conversation

AndrewSisley commented Feb 21, 2024

Relevant issue(s)

Description

codecov bot commented Feb 21, 2024 • edited Loading

Codecov Report

fredcarle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2024 •

edited

Loading

AndrewSisley Feb 23, 2024 •

edited

Loading

AndrewSisley Feb 23, 2024 •

edited

Loading

AndrewSisley Feb 23, 2024 •

edited

Loading

AndrewSisley Feb 27, 2024 •

edited

Loading

AndrewSisley Feb 27, 2024 •

edited

Loading

AndrewSisley Feb 27, 2024 •

edited

Loading

AndrewSisley Feb 27, 2024 •

edited

Loading

AndrewSisley Feb 27, 2024 •

edited

Loading