GH-3708: Inline parquet.thrift - #3709
Conversation
12d885d to
dc2c7b9
Compare
dc2c7b9 to
0c69110
Compare
| tag="apache-parquet-$release_version-rc$2" | ||
|
|
||
| # Ensure the inlined parquet.thrift matches an official parquet-format release. | ||
| "$(dirname "$0")/check-parquet-thrift-release.sh" |
There was a problem hiding this comment.
I think this is an unnecessary requirement. This assumes that we can only release Parquet Java if there has also been a release of Parquet Format, which is beyond the scope of what we are trying to do here.
The primary purpose of this change is to use a local copy of parquet.thrift. A secondary concern is helping us to update that file easily and know when it has changed. I think we should remove changes to the release script unless we decide to make this a requirement.
| # under the License. | ||
|
|
||
| # Verifies that the parquet-format.version sidecar file is tracking a released parquet-format | ||
| # version and that the inlined parquet.thrift byte-matches the upstream file at that version. |
There was a problem hiding this comment.
I think that this should be removed, or at least changed to check against the parquet-format.version file and nothing else. We do need to be able to check the state of the thrift file, but that has little to do with releases. Plus, the file is version controlled, so we should be able to easily tell when it was modified and where the content came from by looking at parquet-format.version.
| commit_arg="" | ||
| version_arg="" | ||
|
|
||
| if [[ "$1" == "--version" ]]; then |
There was a problem hiding this comment.
Can't we just use a ref? If it is a tag then we get that version, otherwise we get the commit hash? That seems simpler than implementing CLI parsing and validation here.
| echo " -> commit: $resolved_sha" | ||
| else | ||
| resolved_sha="$commit_arg" | ||
| resolved_version="UNRELEASED" |
There was a problem hiding this comment.
This isn't necessarily true. I think we should omit the resolved version instead of saying the content is not released. This could be a commit hash referring to a commit that is in a released version.
| echo "ERROR: failed to fetch ${url} after ${attempt} attempts" >&2 | ||
| return 1 | ||
| fi | ||
| sleep $((attempt * 2)) |
There was a problem hiding this comment.
Seems overcomplicated to have retry logic unless we actually need it.
2 participants
Rationale for this change
It is currently not possible to build working POCs for unreleased parquet-format changes in parquet-java because the parquet-format dependency must be updated using a released version. This blocks reference implementations from merging and will become a bigger issue if the current versioning proposal goes through and we start having more "preview" features where writes are expected to support unreleased spec changes behind feature flags.
What changes are included in this PR?
This PR removes the dependency on github.com/apache/parquet-format and instead adds an inlined copy of
parquet.thrift(parquet-format-structures/src/main/thrift/parquet.thrift) as well as a "sidecar" metadata file (parquet-format-structures/src/main/thrift/parquet-format.version) to indicate the parquet-format version that's being inlined. This can be either an X.Y.Z semantic version or a commit hash.There are two scripts added to help manage the vendored copy:
dev/update-parquet-thrift.sh [$SHA | --version X.Y.Zupdates the inlinedparquet.thriftto match an upstream copy of parquet-format given either a commit hash (for POC development) or a version (for releases)dev/check-parquet-thrift-release.shchecks that inlinedparquet.thriftmirrors a parquet-format release, not an unreleased commit. This is meant to be used in parquet-java releases only, not to block features from merging.Are these changes tested?
Edit inlined parquet.thrift with a meaningless change and then validate it gets overwritten:
Also test updating to a commit rather than a version:
Are there any user-facing changes?
No
Closes #3708