Allow StringType and BytesType to have undefined byte lengths #413
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@adrianisk @mjperrone @cpard @gwukelic @alexdemeo PTAL Also, the spec change: recap-build/recap-website#13 |
criccomini
force-pushed
the
make-byte-length-optional
branch
from
cb8860b
to
9db1f36
Compare
mjperrone
approved these changes
Jan 17, 2024
recap/types.py
Outdated
| if not self.variable and self.bytes_ is None: | ||
| raise ValueError("Fixed length bytes must have a length set") | ||
| if self.bytes_ is not None and self.bytes_ < 1: | ||
| raise ValueError("Bytes must be greater than or equal to 1") |
There was a problem hiding this comment.
Why are you dropping the max length constraint here and for Bytes below?
There was a problem hiding this comment.
Mmm, good question. I'm not 100% sure what I was thinking here. The spec still says 64-bit signed integer | null, so if bytes_ is set it should be enforced as such. Good catch.
There was a problem hiding this comment.
Ok, I pushed an updated PR here (force push squash). LMK if this looks OK.
- Went back to LONG_MAX for string/bytes/list length
- Moved list length min from 0 to 1 (in accordance with spec)
- Updated 0.3.0.json to reflect maximum=int_max
- Left 0.3.0.md as is since it already reflected these rules
| @@ -20,7 +20,7 @@ | |||
|
|
|||
| VALID_SCHEMA_DIR = "tests/spec/valid" | |||
| INVALID_SCHEMA_DIR = "tests/spec/invalid" | |||
| RECAP_SPEC_JSON_HTTP = "https://recap.build/specs/type/0.2.0.json" | |||
| RECAP_SPEC_JSON_HTTP = "https://recap.build/specs/type/0.3.0.json" | |||
There was a problem hiding this comment.
I see that this is why the tests are failing, the website change hasn't landed yet. 👍
There was a problem hiding this comment.
Yep! PR for that is here:
criccomini
force-pushed
the
make-byte-length-optional
branch
from
9db1f36
to
859777b
Compare
Prior to this commit, Recap StringType and BytesType both required `bytes_` to be set. If unset, a default of 65536 was used. This configuration allowed developers to skip the `bytes` field in the concrete syntax tree (CST), but still required `bytes_` to be set in the abstract syntax tree (AST). @adrianisk and I converged on this design in #284. Recently, @mjperrone pointed out that requiring `bytes_` in the AST is still awkward for JSON. In the absence of an undefined byte_ option, you have to set *something* for JSON--either a magic number or INT/LONG_MAX. Thus far, we defaulted to LONG_MAX. But a defined LONG_MAX and an undefined length are actually two separate states. Allowing a converter to specify an undefined string/byte length permits other converters to decide how to deal with the undefined byte length. I made the change and it fit quite naturally, so I think we're on the right track with this. JSON and Hive metastore both benefited; both support undefined lengths. I've left the other converters alone for now. This means they'll continue to barf if you go from JSON/HMS to Recap to Avro/Proto. I think that's fine for now. We can revisit this later if we want to.
criccomini
force-pushed
the
make-byte-length-optional
branch
from
859777b
to
aeaf2fa
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Prior to this commit, Recap StringType and BytesType both required
bytes_to be set. If unset, a default of 65536 was used. This configuration allowed developers to skip thebytesfield in the concrete syntax tree (CST), but still requiredbytes_to be set in the abstract syntax tree (AST). @adrianisk and I converged on this design in#284.
Recently, @mjperrone pointed out that requiring
bytes_in the AST is still awkward for JSON. In the absence of an undefined byte_ option, you have to set something for JSON--either a magic number or INT/LONG_MAX. Thus far, we defaulted to LONG_MAX. But a defined LONG_MAX and an undefined length are actually two separate states. Allowing a converter to specify an undefined string/byte length permits other converters to decide how to deal with the undefined byte length.I made the change and it fit quite naturally, so I think we're on the right track with this. JSON and Hive metastore both benefited; both support undefined lengths.
I've left the other converters alone for now. This means they'll continue to barf if you go from JSON/HMS to Recap to Avro/Proto. I think that's fine for now. We can revisit this later if we want to.