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

Simplify the spec to only include the ! nullability designator #13

Open
wants to merge 3 commits into
base: clientControlledNullability
Choose a base branch
from

Conversation

aprilrd
Copy link
Collaborator

@aprilrd aprilrd commented Aug 14, 2023

I've made two changes in this PR:

  1. Remove the ? nullability designator
  2. Relax the validation rule to prevent the designator depth from exceeding the type depth. I realize that it's too restrictive to require a matching designator path with only !.

@aprilrd aprilrd requested a review from twof August 14, 2023 05:23
in a query validation error.
can either use the designator `!` to designate the nullability of the entire
field, or they can use the list element nullability syntax displayed above. The
number of dimensions indicated by list element nullability syntax cannot exceed the number of dimensions of the field. Anything else results in a query
Copy link
Owner

Choose a reason for hiding this comment

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

You may want to have an example where the dimensions of the designator are less than the dimensions of the field type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added some examples after this paragraph.

nullability designator (`!`) can override it for the duration of an execution.
In order to determine a field's true nullability, both are taken into account
and a final type is produced. A field marked with a `!` is called a "required
field".

ApplyRequiredStatus(type, requiredStatus):
Copy link
Owner

Choose a reason for hiding this comment

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

I think I got rid of ApplyRequiredStatus from the execution PR. I think the new function that matters here is called simpleTypeTransfer or something like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed ApplyRequiredStatus, defined simpleTypeTransfer, and added references to simpleTypeTransfer. Can you review the list case of ComputeValue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants