Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: Client Controlled Nullability #895
RFC: Client Controlled Nullability #895
Changes from all commits
8a13c8f
c07d86c
ac03e0e
155e09a
e99db4c
1677f62
ce80aa9
4743051
f392add
1473f78
432555d
1954680
3cc7af5
2782099
6e615bc
0fbd456
f9f5e2f
b24ce32
06819e3
3822c97
159d159
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good reason for the
?
here?This means that something like
field[[]]
is allowed, but doesn't add any value, right? Can it be argued that this may be confusing so it would make more sense to disallow it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
field[[]]
would technically be allowed but do nothing. The?
is there because the nullability operator is optional. If it was not optional then[[!]]?
would not be valid because the second dimension of the list doesn't have a nullability operator.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, I am talking about the
?
inListNullability : [ Nullability? ]
If it were
ListNullability : [ Nullability ]
instead, then[[!]]?
would be valid, right? But[[]]?
wouldn't, which I think is the preferable behavior.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it make sense to allow
[[]?]
though? It could be coded as[?]
but I think it's better to be more explicit (here there's a list of lists, and the inner list is nullable) as that helps people reading the query. I even imagine I'd add lint rules to enforce this for clarity.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @benjie, you are right!
So
ListNullability : `[` Nullability? `]`
does make sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for the community: Is it obvious what's meant by "list dimensions"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more common term 'rank'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
number of dimensions is more like matrixes: M[i, j, k] - dimension 3; Graphq does not have these; what we have is rank (I believe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PostgreSQL uses
dimensions
(and does not mention "rank"):C (and thus probably all C-style languages) uses dimensions; the manual page for arrays doesn't mention the term "rank": https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Multidimensional-Arrays
Haskell seems to use dimensions (this page on arrays doesn't mention "rank"): https://www.haskell.org/tutorial/arrays.html
Fortran uses rank, but defines rank as the "number of dimensions": https://www.ibm.com/docs/en/xffbg/121.141?topic=basics-rank-shape-size-array
.NET uses rank, but quickly defines it as "number of dimensions": https://docs.microsoft.com/en-us/dotnet/api/system.array.rank?view=net-6.0
I think "number of dimensions" is the most universal term, and "rank" is a shorthand used in some languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the .NET link above, note that at least in some languages, jagged arrays, i.e. arrays of arrays that need not be the same length, have rank/dimension of 1 rather than being truly multidimensional. Off the cuff, this is not of major concern, as the rank/dimension referred to seems to be most important there in terms of memory management. Although, I agree that the term "depth" more accurately describes what we have in GraphQL. Perhaps we can define depth even without the use of "dimension" at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this:
?
This way,
0
is just a particular case of a more generic rule that allows "skipping over the list items you want unchanged"With this schema:
This would all be valid:
I don't think it's going to be used that much but it's a more generic rule and avoids "0" as an outlier: everything that's not mentioned explicitely is untouched
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone will need to go back through the notes, but IIRC the concern with this was that it's not clear how to read a list operator with fewer dimensions than the list type it's being applied to. ie
Does
[!] == [[!]]
or does[!] == [[]!]
? @calvincestari will be leading a CCN discussion at the next working group meeting on August 3rd, and we'll also be having a CCN working group meeting on the 26th if you'd like to join that to discuss more: https://github.com/graphql/client-controlled-nullability-wg/blob/main/agendas/2023/2023-07-26.mdThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would work like for a single dimension list.
For this field definition
list1: [String]
,list1!
is the equivalent oflist1[]!
, notlist1[!]
so it's just a generalisation of the "single list" case. Just like! == []!
, I would expect[!] == [[]!]
(modulo the caveat from above about [] but that's something else).I won't be able to join unfortunately but I'm working with Calvin so I'll discuss this with him :)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think
[!] == [[]!]
makes sense.But
If {typeDepth} equals {designatorDepth} or {designatorDepth} equals 0 return
allows future relaxing the rule to beIf {typeDepth} is greater or equal to {designatorDepth} return true
without breaking queries. So would it be possible to advance as is and wait for the feedback from the community?