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.
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: Add compression option ZSTD. #1890
feat: Add compression option ZSTD. #1890
Changes from 7 commits
a61a82e
df23c7d
b26a974
b801aaf
f6da457
b57dd66
867e5bc
3b0f4b0
1af9900
594a2a9
e685c2d
37e7f53
9e1bb36
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how useful this test is? Seems an awful lot like a change-detector test to me. I'd be fine adding the constant without adding the test.
Alternatively, maybe there's a system test we could write to make sure this is synced with the bigquery discovery document? But even then, compression isn't a true enum. The allowed values are only listed in the documentation string from what I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tswast
I will remove this test.
In terms of attempting to ensure that this matches the discovery docs... the terms referenced by the docs are present in the description, as you note, so we would need some mechanism to extract them from the discovery doc, which feels somewhat fragile (ie extract all words that are ALL CAPS and deduplicate them). Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Without a structured representation of the allowed values, it's too fragile.