fix: Make all array kinds nillable #2534

nasdf · 2024-04-19T18:59:37Z

Relevant issue(s)

Resolves #2533

Description

This PR fixes a bug where ScalarArrayKind.IsNillable() returned an incorrect value for arrays of non-nillable values.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

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

MacOS

AndrewSisley

Thanks for finding this :) Got a couple of todos for you though

AndrewSisley · 2024-04-19T19:03:52Z

client/schema_field_description.go

@@ -154,7 +154,11 @@ func (k ScalarArrayKind) Underlying() string {
 }

 func (k ScalarArrayKind) IsNillable() bool {
-	return k == FieldKind_NILLABLE_BOOL_ARRAY ||
+	return k == FieldKind_BOOL_ARRAY ||


todo: This could be simplified to return true.

AndrewSisley · 2024-04-19T19:05:06Z

client/schema_field_description.go

@@ -154,7 +154,11 @@ func (k ScalarArrayKind) Underlying() string {
 }

 func (k ScalarArrayKind) IsNillable() bool {


todo: Please add some integration tests that fail before the production code is fixed.

I'm actually not sure how the tests did not fail before. I'm still investigating, but I believe its because of encoding / decoding of []int{} vs nil.

I've added a unit test, but I don't think an integration test can actually catch this problem due to the encoding I mentioned above.

I believe its because of encoding / decoding of []int{} vs nil.

I think that might be another bug, client.validateFieldSchema does not appear to reject nil values for non-nillable fields. I'll create a new ticket, is kind of out of scope here if that is what was hiding this PR's bug.

AndrewSisley

LGTM, thanks Keenan :)

fredcarle · 2024-04-19T20:34:21Z

client/normal_value_test.go

+// This test documents a bug where array values
+// were not returning the correct value for IsNillable
+// and were also not convertible to a normal nil kind.
+func TestArrayValue_IsNillable(t *testing.T) {


thought: I'm not sure that this is correct. Please wait before merging.

All good here. I got mixed up because of us not updating the field kinds.

fredcarle · 2024-04-19T20:51:35Z

client/normal_nil.go

@@ -34,13 +34,13 @@ func NewNormalNil(kind FieldKind) (NormalValue, error) {
 		return NewNormalNillableString(immutable.None[string]()), nil
 	case FieldKind_NILLABLE_BLOB:
 		return NewNormalNillableBytes(immutable.None[[]byte]()), nil
-	case FieldKind_NILLABLE_BOOL_ARRAY:
+	case FieldKind_BOOL_ARRAY, FieldKind_NILLABLE_BOOL_ARRAY:


todo: This should be changed to

case FieldKind_BOOL_ARRAY: return NewNormalIntNillableArray(immutable.None[[]bool]()), nil case FieldKind_NILLABLE_BOOL_ARRAY: return NewNormalNillableBoolNillableArray(immutable.None[[]immutable.Option[bool]]()), nil

and similarly for the other array types.

Nice spot :)

islamaliev · 2024-04-19T21:20:13Z

client/normal_value_test.go

+		FieldKind_NILLABLE_BOOL_ARRAY,
+		FieldKind_NILLABLE_INT_ARRAY,
+		FieldKind_NILLABLE_FLOAT_ARRAY,
+		FieldKind_NILLABLE_STRING_ARRAY,


todo: sorry, but I'm trying to figure out what this PR actually tries to achieve.
Looking at this test I'm a bit confused.
We have the following array types:

FieldKind__ARRAY - NOT nillable array with NOT nillable elements

FieldKind_NILLABLE__ARRAY - NOT nillable array with nillable elements

FieldKind__NILLABLE_ARRAY - nillable array with NOT nillable elements

FieldKind_NILLABLE__NILLABLE_ARRAY - nillable array with nillable elements
At least this is how these different array kinds are supposed to be distinguished.

At the moment IsNillable on ScalarArrayKind return true IIRC only elements are nillable, but it's no ideal. I also planned to put some effort on this.

So please clarify which of those 4 types you are changing and how.

There was a discord conversation around this, all the scalar arrays are nillable atm, the enum values are just stuck with the old names. e.g. FieldKind_INT_ARRAY is nillable, with non-nillable elements.

At the moment, FieldKind__ARRAY is a Nillable array with NOT nillable elements. That is where we all get confused. This needs to be fix. This PR ensures that things work as they should given the current naming.

FieldKind__NILLABLE_ARRAY and FieldKind_NILLABLE__NILLABLE_ARRAY don't exist at the moment.

looks like I missed this discord conversation. All right then

make all array kinds nillable

8a71b16

nasdf added the bug Something isn't working label Apr 19, 2024

nasdf added this to the DefraDB v0.11 milestone Apr 19, 2024

nasdf self-assigned this Apr 19, 2024

nasdf requested a review from a team April 19, 2024 19:04

AndrewSisley requested changes Apr 19, 2024

View reviewed changes

nasdf added 2 commits April 19, 2024 12:10

simplify ScalarArrayKind IsNillable

2690568

add TestArrayValue_IsNillable

d1737e0

nasdf requested a review from AndrewSisley April 19, 2024 20:17

AndrewSisley approved these changes Apr 19, 2024

View reviewed changes

AndrewSisley mentioned this pull request Apr 19, 2024

client.validateFieldSchema does not error when given nil values for non-nillable fields #2539

Open

fredcarle reviewed Apr 19, 2024

View reviewed changes

islamaliev reviewed Apr 19, 2024

View reviewed changes

nasdf and others added 2 commits April 19, 2024 14:58

use correct normal values for normal nil arrays

8a83889

Merge branch 'develop' into nasdf/fix/array-kind-nillable

d9953a9

nasdf merged commit 39286f1 into sourcenetwork:develop Apr 20, 2024
26 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Make all array kinds nillable #2534

fix: Make all array kinds nillable #2534

nasdf commented Apr 19, 2024

AndrewSisley left a comment

AndrewSisley Apr 19, 2024

AndrewSisley Apr 19, 2024

nasdf Apr 19, 2024

nasdf Apr 19, 2024

AndrewSisley Apr 19, 2024 •

edited

Loading

AndrewSisley Apr 19, 2024

AndrewSisley left a comment

fredcarle Apr 19, 2024

fredcarle Apr 19, 2024

fredcarle Apr 19, 2024 •

edited

Loading

AndrewSisley Apr 19, 2024

islamaliev Apr 19, 2024

AndrewSisley Apr 19, 2024 •

edited

Loading

fredcarle Apr 19, 2024 •

edited

Loading

fredcarle Apr 19, 2024

islamaliev Apr 19, 2024

		@@ -154,7 +154,11 @@ func (k ScalarArrayKind) Underlying() string {
		}

		func (k ScalarArrayKind) IsNillable() bool {

fix: Make all array kinds nillable #2534

fix: Make all array kinds nillable #2534

Conversation

nasdf commented Apr 19, 2024

Relevant issue(s)

Description

Tasks

How has this been tested?

AndrewSisley left a 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 Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

fredcarle Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Apr 19, 2024 •

edited

Loading

fredcarle Apr 19, 2024 •

edited

Loading

AndrewSisley Apr 19, 2024 •

edited

Loading

fredcarle Apr 19, 2024 •

edited

Loading