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 20 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.
Update this once we land on error 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.
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?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.
If folks are cool with it, I think we should be more precise here and say that nulls are propagated rather than errors. I think the language about error propagation likely stems from the GraphQL-JS implementation which centers its logic for propagation decisions around errors, but the errors themselves technically have no impact on any fields beyond causing the originating field to become null, which may in turn violate the spec and cause propagation. The only way parent fields can "handle" an error propagated from a child field is by being
Nullable
.If the intent is to provide fields other means of error handling in a future version of the spec, then I could see a reason to keep the current language.