laskoviymishka commented on code in PR #1094: URL: https://github.com/apache/iceberg-go/pull/1094#discussion_r3269107917
########## table/testdata/geo/gen_fixtures.sh: ########## @@ -0,0 +1,62 @@ +#!/usr/bin/env bash Review Comment: Could we wire this up via `//go:generate` to match the existing convention in the repo? `puffin/gen_dv_fixture.go` + `puffin/dv_golden_test.go` set the pattern for fixture generators, and `exprs.go` does it for stringer — both run via `go generate ./...`. Doesn't have to be a Go program — `//go:generate bash gen_fixtures.sh` is totally fine and keeps the shell script as-is. The win is discoverability: contributors find fixture regeneration through `go generate`, not by spelunking through `table/testdata/geo/` looking for a script. ########## table/testdata/geo/gen_fixtures.sh: ########## @@ -0,0 +1,62 @@ +#!/usr/bin/env bash +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +set -euo pipefail + +# Pin to the specific commit for reproducibility +# If there are changes to the data, this can be bumped +REF="d1f14a06f800238b127b51fef6fa6b9feb15ab0b" +API_URL="https://api.github.com/repos/apache/parquet-testing/contents/data/geospatial?ref=${REF}"; +README="README.md" + +START_MARKER="<!-- BEGIN GENERATED FIXTURE LINKS -->" + +JSON="$(curl -s "$API_URL")" + +# Download all the parquet fixtures, not the associated config files +# or generation scripts +printf '%s\n' "$JSON" \ + | jq -r '.[] | .download_url' \ + | grep '\.parquet$' \ + | xargs -n 1 wget -N Review Comment: Three small portability nits in the script that'll bite the next contributor: - No `cd "$(dirname "$0")"` at the top — running from anywhere else drops files in the caller's cwd - `wget` / `wget -N` isn't installed on macOS by default and is GNU-only; `curl -fLO --time-cond` is the portable equivalent - `curl -s` on line 30 swallows GitHub API rate-limit errors silently — `$JSON` becomes an error envelope, jq returns nothing, and the README gets rewritten with an empty fixture list under `set -e`. `curl -fsS` lets the failure propagate. ########## table/testdata/geo/README.md: ########## @@ -0,0 +1,39 @@ +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +# Geopatial Parquet Fixtures from [parquet-testing](https://github.com/apache/parquet-testing) Review Comment: Nit — "Geopatial" → "Geospatial" (spelled correctly two lines down). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
