Skip to content

Reimplement vec merge - #55

Merged
m-mohr merged 6 commits into
mainfrom
merge-parquet-hydrate
Sep 28, 2026
Merged

m-mohr merged 6 commits into
mainfrom
merge-parquet-hydrate

Conversation

@ivorbosloper

Copy link
Copy Markdown
Collaborator

merge_parquet dropped a property that each part kept as a constant in its collection when the parts disagreed on it: it was neither a column nor in the merged collection. Now it becomes a column again, with each part's value, as vec merge does by reading with hydrate=True. Found in fiboa/cli#338, where the Spanish province code disappeared from the merged file.

  • Only what the merged collection does not carry is hydrated; a constant all parts share stays in the collection. Collection-only properties (title, license, ...) are never hydrated.
  • A part that lacks the property gets NULL; a part that already has it as a column keeps its column.
  • With something to hydrate, the source is one SELECT per part joined with UNION ALL BY NAME; otherwise the query is unchanged.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Fix merge gaps, merge with DuckDB in vec merge
* Merge: hydrate only what the parts disagree on (review #1)
* Merge: keep rows as they are with DuckDB, like in memory (review #2)
* Report constants that don't fit their schema type (review #3)
* Merge: reject two versions of one schema in a collection (review #4)
* DuckDB: key ids by collection only with several collections (review #5)
* Merge: apply excludes to the loaded data in memory (review #6)
* DuckDB: quote required property names with _sql_name (review #7)
* Share the CRS comparison of merge and DuckDB (review #8)
* Merge: warn when includes drop a required collection-only property (review #9)
* Merge: accept a collection that can't be determined (review #10)
* Add strict mode to vec merge, minor improvements and bug fixes (#60)

Co-authored-by: Matthias Mohr <m.mohr@moregeo.it>
Co-authored-by: Ivor <ivorbosloper@gmail.com>
@m-mohr m-mohr changed the title merge_parquet: hydrate the constants the parts disagree on Reimplement vec merge Sep 28, 2026
@m-mohr

m-mohr commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

This PR now also includes a large set of additional changes from PRs #56 #58 and #60.

@m-mohr
m-mohr requested a balanced review from Copilot September 28, 2026 11:54

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

@m-mohr
m-mohr self-requested a review September 28, 2026 15:42
@m-mohr
m-mohr requested a balanced review from Copilot September 28, 2026 15:42

Copilot AI left a comment

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.

Copilot review overview

🔵 Needs a closer look

Existing-target DuckDB merges regress, and required collection-only metadata can evade validation.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Use atomic replacement for DuckDB output to existing targets

vecorel_cli/​merge.py:133

The new automatic DuckDB path regresses merges to an existing target: DuckDB COPY ... TO refuses an existing file by default, while the previous GeoParquet.write path overwrote it. This also breaks using one input as the output path. Write to a temporary sibling and atomically replace the target after success rather than unlinking first, since the target may still be a source.

Medium severity Validate required collection metadata before skipping properties

vecorel_cli/​validation/​geoparquet.py:167

Required collection-only properties are skipped without checking the collection metadata. For example, a non-strict merge that removes a differing required producer leaves its requirement in schemas:custom, but vec validate reports no error here. Check that the metadata contains a non-null value before continuing.

@m-mohr
m-mohr merged commit 9ed1009 into main Sep 28, 2026
8 checks passed
@m-mohr
m-mohr deleted the merge-parquet-hydrate branch September 28, 2026 15:49
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.

3 participants