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(text)!: mark name property of type extension item as required #495

Merged
merged 3 commits into from
Apr 18, 2023

Conversation

mbrobbel
Copy link
Member

@mbrobbel mbrobbel commented Apr 13, 2023

The spec on simple extensions
states:

In the places where these entities are referenced, they will be referenced using a URI + name
reference.

And for type:

Type  | The name as defined on the type object.

However this name field is not marked as required on the type object
definition in the schema. This PR marks this field as required to make sure it
can always be referenced.

@github-actions
Copy link

ACTION NEEDED

Substrait follows the Conventional Commits
specification
for
release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@@ -10,6 +10,7 @@ properties:
items:
type: object
additionalProperties: false
required: [name]
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't structure also be required?

Copy link
Member Author

@mbrobbel mbrobbel Apr 18, 2023

Choose a reason for hiding this comment

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

Marking that field as required results in:

unknown.yaml invalid
[
  {
    instancePath: '/types/0',
    schemaPath: '#/properties/types/items/required',
    keyword: 'required',
    params: { missingProperty: 'structure' },
    message: "must have required property 'structure'"
  }
]

Copy link
Member

Choose a reason for hiding this comment

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

Got it. I found this paragraph on the site:

The structure field is optional. If not specified, the type class is considered to be fully opaque. This implies that a systems without built-in support for the type cannot manipulate values in any way, including moving and cloning. This may be useful for exotic, context-sensitive types, such as raw pointers or identifiers that cannot be cloned.

Sorry for the goose chase

@mbrobbel mbrobbel changed the title fix(text)!: mark name property of type extension item as required fix(text)!: mark name and structure property of type extension item as required Apr 18, 2023
@mbrobbel mbrobbel force-pushed the type-extension-name-required branch from 2309a73 to 3192890 Compare April 18, 2023 08:41
@mbrobbel mbrobbel changed the title fix(text)!: mark name and structure property of type extension item as required fix(text)!: mark name property of type extension item as required Apr 18, 2023
@westonpace westonpace merged commit 7246102 into substrait-io:main Apr 18, 2023
@mbrobbel mbrobbel deleted the type-extension-name-required branch April 18, 2023 14:16
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