Review fixes for #56 - #58
Merged
Merged
Conversation
The in-memory merge hydrated every constant and relied on the dehydration on write to move them back, which fails for arrays and objects. It now merges the collections first and hydrates only the keys the merged collection drops, as merge_parquet does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
vec merge only appends datasets, so the DuckDB engine no longer applies the converter checks there: a missing required value is reported with a merge-appropriate warning instead of an error with converter advice, and empty geometries are kept, as the in-memory merge does. This is a new strict flag on write_query that vec merge turns off; converters and merge_parquet itself keep the checks. The per-collection null condition no longer repeats the collection check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_constants_table caught every exception and fell back to an inferred type silently. It now only catches the conversion errors and warns with the property and its type; a schema without a pyarrow type still falls back without a warning. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
add_all only rejected conflicting core versions; two versions of the same extension (e.g. fiboa v0.2.0 and v0.3.0) in one collection were kept side by side. Schemas that only differ in their version segment are now rejected the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The id uniqueness check and the suffixing used a (collection, id) key whenever there is a collection column, which slows down the common single-collection conversion for nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With --exclude, every dataset without a cheap property listing (e.g. GeoJSON) was read in full to get its columns and then read again by the merge. The in-memory merge now takes the excludes and resolves them on the data it has loaded; the DuckDB path still lists the GeoParquet columns, which only reads the schema. A merge without the collection no longer adds it back or needs it for the duplicate check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
get_duckdb_blocker re-implemented the loop of DuckDBBaseConverter._common_crs with its private helpers. Both now use find_differing_crs from vecorel.util. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eview #9) warn_missing_required skipped every collection-only property, but merge_collections drops those that are not in the included properties, so a required one disappeared without a warning. Only the ones the merged collection still carries are skipped now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
get_collection_id raised for rows without a collection when the metadata doesn't name one, so create-geoparquet and the in-memory merge failed on input they used to accept, while merge_parquet caught it in one place only. It now returns None and both merges leave such rows as they are. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Thanks for the check. With regards to the points, I have a hard time understanding them from the issue so will have to look at the commits. Will probably take me a bit... All commits and the PR text refer to issues, but I think these are not the issues that are actually relevant here. It seems Claude just slopped # in front of all numbers without being aware of GitHub. Removed it from the text, but not from the commits. |
m-mohr
marked this pull request as ready for review
September 28, 2026 09:37
Contributor
|
Generally looks good. I think it might be worthwhile to add a strict/non-strict mode to vec merge, which both could be useful depending on the usecase. Will create a separate PR that stack on top of this. |
m-mohr
added this pull request to stack #57
September 28, 2026 11:21
m-mohr
removed this pull request from stack #57
September 28, 2026 11:39
* Add strict mode to vec merge, minor improvements and bug fixes * Fix typed constants, missing geometries, nested nulls and schema map in merge
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
2 participants
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.
Code for the review findings on #56, one commit per finding, so they can be picked individually.
hydrate_from_collectiontakeskeys), so shared array constants stay in the collection.write_querygets astrictflag thatvec mergeturns off: a missing required value is a warning instead of an error with converter advice, and empty geometries are kept, as in memory. Converters andmerge_parquetkeep the checks. The per-collection null condition no longer repeats the collection check._constants_tableonly catches conversion errors and warns with the property and its type.Schemas.add_allrejects two versions of one schema in a collection, like core versions.excludesand resolves them on the data it loaded; only the DuckDB path lists properties, from the GeoParquet schemas._sql_name.find_differing_crsinvecorel.utilreplaces the CRS loops of_common_crsandget_duckdb_blocker.warn_missing_requiredonly skips collection-only properties that the merged collection still carries.get_collection_idreturns None instead of raising; both merges leave such rows as they are.🤖 Generated with Claude Code