feat: add Iceberg read support via source_format - #1100
Conversation
1b722c2 to
a937950
Compare
gadomski
left a comment
gadomski
left a comment
There was a problem hiding this comment.
This is great, thanks! One refactor (using a setter), then some nit-picky suggestions to remove inline comments and a couple other tweaks — thanks for tolerating my nits 🙇🏼!
There was a problem hiding this comment.
I don't think we need two scripts, we can use a #!/usr/bin/env python shebang on this script and put the Python here.
| import pyarrow as pa | ||
| import pyarrow.parquet as pq |
There was a problem hiding this comment.
Nit: I know most folks (and their docs) use these abbreviations, but I prefer to just use the full names — letters are cheap 😄
|
|
||
|
|
||
| def read_subset() -> pa.Table: | ||
| table = pq.read_table(SOURCE_PARQUET, columns=["id", "datetime", "collection", "eo:cloud_cover", "platform", "geometry"]).slice(0, 5) # Take 5 rows for the fixture |
There was a problem hiding this comment.
The slice is clear enough, no comment needed.
| table = pq.read_table(SOURCE_PARQUET, columns=["id", "datetime", "collection", "eo:cloud_cover", "platform", "geometry"]).slice(0, 5) # Take 5 rows for the fixture | |
| table = pq.read_table(SOURCE_PARQUET, columns=["id", "datetime", "collection", "eo:cloud_cover", "platform", "geometry"]).slice(0, 5) |
|
|
||
| def main() -> None: | ||
| build_dir = FIXTURE_DIR.parent / "build" | ||
| # Checks for previous builds and deletes if so |
There was a problem hiding this comment.
| # Checks for previous builds and deletes if so |
| table_dir = build_iceberg_table(build_dir, data) | ||
| rewrite_paths(table_dir, FIXTURE_REL) | ||
|
|
||
| # Move the build to the fixture location and clean up the build dir |
There was a problem hiding this comment.
| # Move the build to the fixture location and clean up the build dir |
| connection.execute("INSTALL iceberg", [])?; | ||
| connection.execute("LOAD iceberg", [])?; |
There was a problem hiding this comment.
I don't think we want to install iceberg unless we need it — maybe we make source_format a private attribute and use a setter to install the extension if the source-format is iceberg?
This has the added benefit of making this change non-API-breaking (adding a public attribute to a struct is a breaking change).
| href: &str, | ||
| search: Search, | ||
| ) -> Result<SearchArrowBatchIter<'conn>> { | ||
| self.apply_source_format_settings()?; |
There was a problem hiding this comment.
Do we need this? Isn't this done in build_query?
| /// `enable_geoparquet_conversion` is disabled for Iceberg to avoid schema | ||
| /// mismatch and subsequent crash. For Parquet, the setting is reset to the default. | ||
| fn apply_source_format_settings(&self) -> Result<()> { | ||
| match self.source_format { | ||
| SourceFormat::Iceberg => { | ||
| self.execute("SET enable_geoparquet_conversion = false", [])?; | ||
| } | ||
| SourceFormat::Parquet => { | ||
| self.execute("RESET enable_geoparquet_conversion", [])?; | ||
| } | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
Per the comment above, if we use a getter/setter for source_format, we can put this operation in that setter.
2 participants
Closes #942
Description
Add support for reading Iceberg via DuckDB. Driven by a new enum to choose between source format Parquet and Iceberg. Some hiccups around default
enable_geoparquet_conversionas Iceberg (v2) declares geometrybinarybut DuckDB transforms to nativegeometryper geoparquet 1.2 (seeapply_source_format_settings).Checklist
prek run --all-files)