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

feat: remove property reservations #468

Closed
wants to merge 1 commit into from
Closed

Conversation

nrwiersma
Copy link
Member

Parsing no longer necessitates having reservations when adding properties, further this allows users more freedom in what properties are stored, as long as they do not conflict with the current types fields.

@jhump
Copy link
Contributor

jhump commented Oct 11, 2024

A few thoughts:

  1. This seems too permissive. For example, it now allows NewArraySchema to be called with a property named "type" or "items", which will almost certainly produce invalid JSON.
  2. This doesn't quite resolve all of the issues with preserving unrecognized logical types (or preserving "precision" and "scale" properties for types that are not decimal types or are invalid decimal types).
  3. This doesn't preserve any properties for null types (which should not be an exception: the spec describes them in the same breath as the other primitive types).

What do you think of this instead? jhump/avro@jh/improve-ocf-encoder...jhump:avro:jh/preserve-unrecognized
In particular, the new TestParse_PreservesAllProperties might be good to run in this branch to see what I mean by bullets two and three.

In an ideal world, I think use of a reserved property would result in the factory function returning an error. However this is not backwards compatible since NewPrimitiveSchema, NewArraySchema, and NewMapSchema are unable to return an error (without incompatible change to their signature). Alas...

@nrwiersma
Copy link
Member Author

@jhump Please open this as a PR to superceded this.

Just a note, in the parser, it should not be needed to add LogicalType where it does not naturally exist, and it will be added to Props as part of the ,remain.

@jhump
Copy link
Contributor

jhump commented Oct 11, 2024

@jhump Please open this as a PR to superceded this.

Done: #469

@nrwiersma
Copy link
Member Author

Superseded by #469

@nrwiersma nrwiersma closed this Oct 11, 2024
@nrwiersma nrwiersma deleted the prop-reservations branch October 11, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants