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

Adds Ion Schema 2.0 specification #82

Merged
merged 12 commits into from
Sep 22, 2022
Merged

Conversation

popematt
Copy link
Contributor

@popematt popematt commented Sep 13, 2022

Issue #, if available:

Fixes #76

Description of changes:

  • Adds a BNF-style grammar
  • Adds a prose description of Ion Schema Language 2.0

Examples are still TODO because I would like to put the examples into ion-schema-tests and then embed them into the spec from there.

You can see the rendered view of these changes at https://popematt.github.io/ion-schema/docs/isl-2-0/spec

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
- `$lob`: represents a `$blob` or `$clob`
- `lob`: represents a `blob` or `clob`
- `$number`: represents a `$decimal`, `$float`, or `$int`
- `number`: represents a `decimal`, `float`, or `int`

Choose a reason for hiding this comment

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

... that is non-null. Maybe a little refactoring of this section would avoid needing to respecify that.

Choose a reason for hiding this comment

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

More broadly... I read the writeup about the potential ambiguities with nullable in 1.0 and my recollection from working with it was that it was confusing. So I agree that we can and should do better with 2.0.

I also agree that the solution should be conceptually sound and it should be reasonably obvious how to do the common things. I can see how the model here reflects the ion type system, but I suspect that users will find it confusing, for two reasons: remembering presence or absence of '$' sigil (consider something like Kotlin's '?'), and my experience is that most users, when something is null, don't actually care if it's strongly typed or not.

Were there some other models that were considered and rejected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a little refactoring of this section would avoid needing to respecify that

Sure, I can do that.

Were there some other models that were considered and rejected?

Does this question refer to other models for the built-in types in general, or does it refer specifically to the replacement for nullable?

Choose a reason for hiding this comment

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

Replacement for nullable.

Copy link
Contributor Author

@popematt popematt Sep 14, 2022

Choose a reason for hiding this comment

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

Yes. Based on the goal of finding a replacement that was explicit about what would be null (ie. no figuring out "base types"), we came up with the following alternatives that were considered:

  1. nullable field in type definitions that define which typed nulls are allowed with the nullable annotation. This was rejected for being "awkward" and also because it looks like a constraint but behaves unlike any other constraint. The nullable field could also be on a type that is imported from another file, making it more difficult for a human reader to discover what sort of null would be allowed.
  2. nullable:: annotation changed to always allow any sort of null at all. This was rejected because it would be weird/unintuitive for nullable::struct to allow a null.int—it doesn't properly support any use case that people would want to use nullable for.
  3. Get rid of nullable entirely. This was considered the second-best option, but everyone who was consulted agreed that nullability is a common enough use case that we should provide some sort of syntactical sugar for it.
  4. Replace nullable with nullable_text, nullable_string, nullable_struct, etc. annotations to indicate which types of nulls should be allowed. This was rejected for being more complex than what seemed necessary. However, rejecting this is not a one-way door with respect to the chosen solution. It would be possible in Ion Schema 2.1 to add annotations like null_string_or::, etc. so that there is syntactical sugar for all of the strongly-typed nulls.

The `$null_or` annotation may not be added to top-level type definitions; it is only applicable to type references.
When the `$null_or` annotation is present on any type reference, it SHALL be evaluated equivalently to the union of `$null` and the annotated type.

For example, to allow `null` or any non-null integer value, you would use `$null_or::int`.

Choose a reason for hiding this comment

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

I'd be curious how often folks want to constrain to allow untyped but not typed null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't know, nor do we have a good way of knowing.

my experience is that most users, when something is null, don't actually care if it's strongly typed or not

I agree, but I would add I think users benefit from having a single representation of null within their application. So they might not care if it's a strongly typed null or not, but they don't want a mix of the two. It's like indenting code—in many languages you can indent with 2 spaces or 4 spaces (we're ignoring tab characters). What matters most is not how much white space is used for indenting, but that the amount of whitespace is consistent.

docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Show resolved Hide resolved
Copy link
Contributor

@desaikd desaikd left a comment

Choose a reason for hiding this comment

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

Overall looks good!
I like all the notes for constraint sections, those seem very helpful to me. I have added some minor suggestions.

docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
@popematt popematt requested a review from desaikd September 19, 2022 19:49
docs/isl-2-0/spec.md Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved

Ion Schema has several built-in types that are implicitly imported before any other imports are handled.

The nominal Ion types are prefixed with `$`, and correspond precisely with the types defined by the Ion data model, including strongly-typed null values:

Choose a reason for hiding this comment

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

correspond precisely with the types defined by the Ion data model, including strongly-typed null values

I find this (pre-existing) sentence is a bit muddy. I think it means that (e.g.) $int represents the union of [int and null.int] and does NOT include null/null.null. If that's the case, I think we should spell it out.

docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
popematt and others added 3 commits September 20, 2022 10:23
Co-authored-by: Zack Slayton <[email protected]>
Co-authored-by: Zack Slayton <[email protected]>
Co-authored-by: Zack Slayton <[email protected]>
Copy link

@zslayton zslayton left a comment

Choose a reason for hiding this comment

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

Nice work, Matt!

docs/isl-2-0/bnf-grammar.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
docs/isl-2-0/spec.md Outdated Show resolved Hide resolved
Comment on lines 695 to 696
q: "What is the biggest type of penguin?",
a: "The emperor penguin."
Copy link

Choose a reason for hiding this comment

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

For accuracy, consider "biggest extant species of penguin". We don't want to anger fans of the late Colossus penguin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🐧 👍

Copy link

@tgregg tgregg left a comment

Choose a reason for hiding this comment

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

Nice.

@popematt popematt merged commit 8671e3b into amazon-ion:gh-pages Sep 22, 2022
@net-yehor-tretiakov
Copy link

Hey! Esteemed ion-schema contributors, and especially @popematt, I have just started working on a new project that will use ion as encoding/decoding. And i need to write some codegen for different langs depend on .isl files.
Would you mind sharing with me an ion-schema ebnf or smth like ANTLR4 full grammar? I saw https://amazon-ion.github.io/ion-schema/docs/isl-2-0/bnf-grammar, but it cannot be used due to obvious reasons.
Basically, i need to write some kind of schema-lang file parser at first.

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.

Add Ion Schema 2.0 Specification to Ion Schema website
8 participants