Reimplement vec merge - #55
Open
ivorbosloper wants to merge 6 commits into
Open
ivorbosloper wants to merge 6 commits into
ivorbosloper wants to merge 6 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ivorbosloper
marked this pull request as ready for review
September 26, 2026 09:40
2 tasks
m-mohr
added this pull request to stack #57
September 26, 2026 10:58
This was referenced Sep 26, 2026
m-mohr
removed this pull request from stack #57
September 28, 2026 11:39
* 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
changed the title
vec merge
Contributor
m-mohr
approved these changes
Sep 28, 2026
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The in-memory engine still drops selected all-null columns, producing inconsistent schemas and violating non-strict behavior.
Review effort: Balanced
Findings: 1
Open (1)
Resolved since last review (2)
Comment on lines
+115
to
+119
| # Remove empty columns, except for the geometry, which is required | ||
| geometry = merged.geometry.name | ||
| merged = merged.drop( | ||
| columns=[c for c in merged.columns if c != geometry and merged[c].isna().all()] | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
merge_parquetdropped 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, asvec mergedoes by reading withhydrate=True. Found in fiboa/cli#338, where the Spanish province code disappeared from the merged file.NULL; a part that already has it as a column keeps its column.SELECTper part joined withUNION ALL BY NAME; otherwise the query is unchanged.🤖 Generated with Claude Code