Add DuckDB-based converter by m-mohr · Pull Request #29 · vecorel/cli

m-mohr · 2026-09-11T14:13:15Z

Migrated the DuckDB converter from vecorel to fiboa. Also, ensuring outputs are comparable and validate.

Copilot

🟡 Changes recommended

Several paths can emit noncompliant or corrupted output, and the full-memory Hilbert sort defeats large-dataset scalability.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds an experimental DuckDB-based conversion path and shared GeoParquet normalization and Hilbert-ordering support.

Changes:

Adds DuckDB conversion for large Parquet datasets.
Adds GeoParquet post-processing and Arrow type normalization.
Adds converter validation safeguards and parity tests.

File summaries

File	Description
`vecorel_cli/vecorel/hilbert.py`	Adds Parquet Hilbert sorting utilities.
`vecorel_cli/parquet/types.py`	Adds Arrow type normalization.
`vecorel_cli/encoding/geoparquet.py`	Adds GeoParquet post-processing.
`vecorel_cli/encoding/base.py`	Adds the post-processing interface.
`vecorel_cli/conversion/duckdb.py`	Implements DuckDB conversion.
`vecorel_cli/conversion/base.py`	Adds converter checks and row cleanup.
`tests/test_hilbert.py`	Tests Hilbert helpers.
`tests/test_encoding_geoparquet.py`	Tests post-processing.
`tests/test_convert_duckdb.py`	Tests conversion and parity.
`pyproject.toml`	Adds DuckDB dependency.
`pixi.lock`	Locks DuckDB packages.
`CHANGELOG.md`	Documents the changes.

Review details

Suppressed comments (2)

vecorel_cli/encoding/geoparquet.py:401

Optional fields inherit field.nullable=False from the external file, so this can leave them non-nullable even though the Vecorel schema requires optional fields to be nullable; GeoParquetValidator then rejects the supposedly postprocessed file. Derive nullability from the merged schema rather than preserving stricter source nullability.

                pa.field(
                    field.name,
                    pa_type,
                    nullable=field.nullable and field.name not in required_columns,
                    metadata=field.metadata,

vecorel_cli/conversion/duckdb.py:167

Swallowing post-processing failures allows convert() to return success with the raw DuckDB file, even though its types, nullability, bbox, or metadata may be noncompliant. This is a required conversion stage, so propagate the exception (cleanup is already handled by postprocess) instead of warning and continuing.

        except Exception as e:
            self.warning(f"GeoParquet post-processing failed: {e}")

Files reviewed: 11/12 changed files
Comments generated: 8
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🔵 Needs a closer look

Missing-column filters can fail conversion, schema retries delay permanent failures, and explicit compression levels can be ignored.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

vecorel_cli/conversion/base.py:219

This retries every exception, including permanent failures such as a missing local schema or malformed JSON/YAML. Those cases now sleep for roughly three minutes before returning the same deterministic error. Restrict retries to transient network/I/O failures and fail immediately for parsing/configuration errors.
vecorel_cli/conversion/duckdb.py:190
column_filters are keyed by source column, but this path appends every filter even when that source column is absent. Unlike BaseConverter.filter_rows (which warns and skips at conversion/base.py:408-426), this produces a DuckDB binder error for optional/missing source fields. Check k in available before adding the SQL fragment and emit the same warning otherwise.
vecorel_cli/encoding/geoparquet.py:325
An explicitly requested compression_level is ignored when the codec and schema already match: compression_changed remains false, _rewrite returns None, and the file is never recompressed. Track whether the caller supplied a level and force a rewrite in that case (compression levels are not recoverable from Parquet metadata).

Files reviewed: 11/12 changed files
Comments generated: 0 new
Review effort level: Balanced

ivorbosloper

Yes, all lessons learned from fiboa/cli#198 and fiboa/cli#214 are in here

A source that is already Parquet does not need to become a GeoDataFrame to be converted. This converter maps columns, filters rows and migrates values in SQL, writes the result with DuckDB, and then repairs what DuckDB does not write itself: the GeoParquet metadata, the collection metadata, the nullability the schema asks for, and the bbox covering column, before sorting the file into the canonical Hilbert order. Japan's Fude polygons are 33 GB of Parquet across four editions, which is what this was written for; the memory-bound path would need the whole edition in memory. This module is temporary. The same converter is moving into vecorel-cli in vecorel/cli#29, which already carries everything learned here — the Hilbert fallback for a CRS with no area of use, the large_binary widening for takes over 2 GB, the metadata-preserving batch write, the bbox covering, the Windows file handles. When that lands and is released, this file should be deleted and its one user re-pointed at the upstream class. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVx9uQV2QPM8ecPAY3ZjXG

The base converter concatenates every source file into one frame before it migrates anything, so peak memory is the whole dataset: roughly 1.45 GB per million features held. Spain's FEGA release is 52 provincial files and does not fit. PerFileBaseConverter migrates and writes each file on its own, then merges the parts into the output and removes them. The merge keeps the GeoParquet version and the schema metadata of the parts, and the result is sorted into the canonical Hilbert order — over the merged extent, so the ordering does not depend on which file a feature came from. `hilbert_reference_bounds` and `_ensure_hilbert_sorted` live here for now. They belong in vecorel-cli (they are in vecorel/cli#29), and this module should lose them when that lands. Two smaller things the Spanish converter needs: absolute ec_mapping_csv URLs go through the same loader as EuroCrops' relative ones, and a mapping that produces a single distinct HCAT code is no longer treated as a broken mapping — one crop is what a single province looks like. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVx9uQV2QPM8ecPAY3ZjXG

A source that is already Parquet does not need to become a GeoDataFrame to be converted. This converter maps columns, filters rows and migrates values in SQL, writes the result with DuckDB, and then repairs what DuckDB does not write itself: the GeoParquet metadata, the collection metadata, the nullability the schema asks for, and the bbox covering column, before sorting the file into the canonical Hilbert order. Japan's Fude polygons are 33 GB of Parquet across four editions, which is what this was written for; the memory-bound path would need the whole edition in memory. This module is temporary. The same converter is moving into vecorel-cli in vecorel/cli#29, which already carries everything learned here — the Hilbert fallback for a CRS with no area of use, the large_binary widening for takes over 2 GB, the metadata-preserving batch write, the bbox covering, the Windows file handles. When that lands and is released, this file should be deleted and its one user re-pointed at the upstream class. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVx9uQV2QPM8ecPAY3ZjXG

The base converter concatenates every source file into one frame before it migrates anything, so peak memory is the whole dataset: roughly 1.45 GB per million features held. Spain's FEGA release is 52 provincial files and does not fit. PerFileBaseConverter migrates and writes each file on its own, then merges the parts into the output and removes them. The merge keeps the GeoParquet version and the schema metadata of the parts, and the result is sorted into the canonical Hilbert order — over the merged extent, so the ordering does not depend on which file a feature came from. `hilbert_reference_bounds` and `_ensure_hilbert_sorted` live here for now. They belong in vecorel-cli (they are in vecorel/cli#29), and this module should lose them when that lands. Two smaller things the Spanish converter needs: absolute ec_mapping_csv URLs go through the same loader as EuroCrops' relative ones, and a mapping that produces a single distinct HCAT code is no longer treated as a broken mapping — one crop is what a single province looks like. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVx9uQV2QPM8ecPAY3ZjXG

A source that is already Parquet does not need to become a GeoDataFrame to be converted. This converter maps columns, filters rows and migrates values in SQL, writes the result with DuckDB, and then repairs what DuckDB does not write itself: the GeoParquet metadata, the collection metadata, the nullability the schema asks for, and the bbox covering column, before sorting the file into the canonical Hilbert order. Japan's Fude polygons are 33 GB of Parquet across four editions, which is what this was written for; the memory-bound path would need the whole edition in memory. This module is temporary. The same converter is moving into vecorel-cli in vecorel/cli#29, which already carries everything learned here — the Hilbert fallback for a CRS with no area of use, the large_binary widening for takes over 2 GB, the metadata-preserving batch write, the bbox covering, the Windows file handles. When that lands and is released, this file should be deleted and its one user re-pointed at the upstream class. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVx9uQV2QPM8ecPAY3ZjXG

vecorel-cli 0.2.18 carries the SQL converter this repository had a copy of (vecorel/cli#29), so jp imports DuckDBBaseConverter and the copy can go. It also carries the checks the structural PRs here were proposing — rows that cannot validate dropped under max_dropped_share, the required properties read from the declared schemas rather than a hardcoded list, unique ids, no converter declaring both sources and variants, schemas fetched up front. Those checks fire on two converters as soon as the pin moves, so their fixes come along: hr declared a rolling `sources` beside its variants, which made every --variant convert the current file, and Europe-LAND's LT 2024 ships an empty crop_code beside a populated crop_name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVx9uQV2QPM8ecPAY3ZjXG

Add DuckDB-based converter

4ceea9d

m-mohr requested review from ivorbosloper and a balanced review from Copilot September 11, 2026 14:13

Copilot started reviewing on behalf of m-mohr September 11, 2026 14:13 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

m-mohr requested a balanced review from Copilot September 11, 2026 14:37

Copilot started reviewing on behalf of m-mohr September 11, 2026 14:38 View session