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: Make all array kinds nillable #2534

Merged
merged 5 commits into from
Apr 20, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions client/normal_value_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1622,3 +1622,28 @@ func TestNormalValue_ToArrayOfNormalValues(t *testing.T) {
})
}
}

// 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

fieldKinds := []FieldKind{
FieldKind_BOOL_ARRAY,
FieldKind_INT_ARRAY,
FieldKind_FLOAT_ARRAY,
FieldKind_STRING_ARRAY,
FieldKind_NILLABLE_BOOL_ARRAY,
FieldKind_NILLABLE_INT_ARRAY,
FieldKind_NILLABLE_FLOAT_ARRAY,
FieldKind_NILLABLE_STRING_ARRAY,
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. FieldKind__ARRAY - NOT nillable array with NOT nillable elements
  2. FieldKind_NILLABLE__ARRAY - NOT nillable array with nillable elements
  3. FieldKind__NILLABLE_ARRAY - nillable array with NOT nillable elements
  4. 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.

Copy link
Contributor

@AndrewSisley AndrewSisley Apr 19, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@fredcarle fredcarle Apr 19, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FieldKind__NILLABLE_ARRAY and FieldKind_NILLABLE__NILLABLE_ARRAY don't exist at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like I missed this discord conversation. All right then

}

for _, kind := range fieldKinds {
assert.True(t, kind.IsNillable())

v, err := NewNormalNil(kind)
require.NoError(t, err)

assert.True(t, v.IsNil())
}
}
Loading