Skip to content

test(table): geospatial test fixtures#1094

Merged
laskoviymishka merged 5 commits into
apache:mainfrom
C-Loftus:addGeoTestFixtures
May 19, 2026
Merged

test(table): geospatial test fixtures#1094
laskoviymishka merged 5 commits into
apache:mainfrom
C-Loftus:addGeoTestFixtures

Conversation

@C-Loftus
Copy link
Copy Markdown
Contributor

@C-Loftus C-Loftus commented May 19, 2026

  • Add geospatial test fixtures that can be used for validating geospatial logic performs as expected. All fixtures come from the parquet-testing repository. I was previously unaware this repo existed but I think this is the right data to use since it prevents us from needing to write custom pyarrow code to generate them.

    • I put all generation logic in a shell script since it's a bit more succinct than a custom script in go and I don't imagine it will need to be run very often given the last update of that test data was over a year ago.
  • Note these fixtures use all the native parquet geometry type not the geoarrow encoding. I am pretty sure this is what we want, but if I am misinterpreting something let me know. My understanding is that geoarrow is not used in the native parquet type (but from reading other PRs like it seems like geoarrow-go is being used for some of the parse logic, not the storage encoding?) There is more data here https://github.com/geoarrow/geoarrow-data if we need more, some of which is _native.parquet but since it does include a lot of geoparquet files with the geoarrow encoding, I did not use any of these.

  • I have not added any parse tests for this since my understanding is that the initial geospatial PRs here have not been merged yet. Given these are standard parquet test files, it can be assumed they will need to be able to be parsed and conform to the spec.

Closes #1090

@C-Loftus C-Loftus requested a review from zeroshade as a code owner May 19, 2026 13:44
@C-Loftus C-Loftus changed the title Add geospatial test fixtures test(table): geospatial test fixtures May 19, 2026
Copy link
Copy Markdown
Contributor

@laskoviymishka laskoviymishka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixtures look good to me — pinned to a parquet-testing SHA, ASLv2 on both sides, deterministic contents. I’m fine landing them before the parser.

One ask before merge: can we wire regeneration through //go:generate? puffin/gen_dv_fixture.go + puffin/dv_golden_test.go is the closest precedent. No need to rewrite the generator in Go, just //go:generate bash gen_fixtures.sh is fine. This just makes fixture refresh discoverable via go generate ./....

I left a couple small portability nits inline, plus one README typo: GeopatialGeospatial.

Thanks again for picking this up!

Comment thread table/testdata/geo/README.md Outdated
Comment thread table/testdata/geo/gen_fixtures.sh
Comment thread table/testdata/geo/gen_fixtures.sh Outdated
} >> "$README" No newline at end of file
} >> "$README"

echo "Finished fetching parquet fixtures with geo data from parquet-testing repo" No newline at end of file
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a little message here so it is clear what happened when a user runs go generate

Comment thread table/gen_geo_fixtures.go
Comment on lines +17 to +22

package table

// Run the following command to generate the parquet fixtures with
// geo data for testing purposes: go generate ./table/...
//go:generate bash testdata/geo/gen_fixtures.sh
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put this in the table package since there is not a geo package. Realistically once the geo package is merged and stabilized it may make more sense to put it there. Once that is merged feel free to ping me and I am happy to switch. Just didn't want to create an empty package for no reason that could cause merge conflicts with other open PRs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, totally make sense as follow up later

@C-Loftus C-Loftus requested a review from laskoviymishka May 19, 2026 20:37
@C-Loftus
Copy link
Copy Markdown
Contributor Author

Should be all set for review again now, thanks for feedback!

Copy link
Copy Markdown
Contributor

@laskoviymishka laskoviymishka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: , nice work!

@laskoviymishka laskoviymishka merged commit c131e0e into apache:main May 19, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(table) Geospatial Test Fixtures

2 participants