Review fixes for #56

CHANGELOG.md

@@ -13,30 +13,39 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   without loading them into memory; `--engine` chooses the engine explicitly.
 - `vec merge --crs first` keeps the CRS of the first dataset, the default is still EPSG:4326.
 - `merge_parquet` accepts `properties` to restrict the merged properties.
+- `vec merge --no-strict` warns instead of failing if the merged dataset would be invalid
+  and writes it anyway; `merge_parquet` has a `strict` parameter (default: `True`).
+  See the README for the differences between the modes.
 
 ### Changed
 
 - **Breaking:** `vec merge` keeps all properties by default, `--include` restricts them to
-  the core properties plus the given ones and `--exclude` removes any property. It warns if
-  a required property is not included.
+  the core properties plus the given ones and `--exclude` removes any property. It reports
+  required properties that are not included, also collection-only ones.
+- **Breaking:** `vec merge` is strict by default: it fails if the merged dataset would be
+  invalid, e.g. because an id repeats within a collection.
+- The message for missing required values names the missing properties with counts,
+  instead of showing the SQL condition.
 
 ### Fixed
 
 - `merge_parquet` no longer drops a property that each part kept as a constant in
-  its collection but on which the parts disagree; it becomes a column again, as in
-  `vec merge`.
+  its collection but on which the parts disagree; it becomes a column again, with the
+  data type of its schema, as in `vec merge`.
 - Merging no longer overwrites the schemas of a collection that occurs in multiple
-  datasets, it unites them.
+  datasets, it unites them and rejects two versions of the same schema in one collection.
 - Merging warns about collection-only properties that differ between the datasets
   and have to be removed.
 - Merging fills in missing collection values of datasets that only list a single
   collection in `schemas`.
-- Merging hydrates array and object constants correctly, instead of spreading them
-  over the rows, dropping them (`vec merge`) or failing (`merge_parquet`).
+- `vec merge` handles constants correctly: arrays and objects are no longer spread over
+  the rows or dropped, constants that all datasets share stay in the collection, and
+  constants get the data type of their schema, e.g. binary values are decoded instead
+  of written as base64 text.
+- Merging reports a constant that doesn't fit the data type of its schema, with the
+  property and the file, instead of failing with an Arrow error.
 - `merge_parquet` checks and numbers repeating ids per collection instead of across
   all collections, and checks the required properties per collection.
-- `merge_parquet` hydrates date-time, date and binary constants with their types and
-  NaN as null.
 - `merge_parquet` recomputes the bbox, which was empty for the rows of GeoParquet 1.0 parts.
 - Columns without any value are no longer moved to the collection, which wrote NaN
   (invalid JSON) or failed for nullable integers.

README.md

@@ -132,6 +132,33 @@ Local GeoParquet files that are all in the target CRS are merged with DuckDB, so
 All other datasets (e.g. GeoJSON or datasets that need to be reprojected) are merged in memory.
 Use `--engine` to choose the engine explicitly.
 
+`-i` and `-e` apply to all properties, including collection-level metadata.
+Constants that differ between the datasets are moved from the collection metadata to the features.
+
+#### Strict and non-strict mode
+
+By default, `vec merge` is strict: problems that make the merged dataset invalid are errors
+and no dataset is written.
+With `--no-strict`, `vec merge` is fail-safe: these problems are reported as warnings and
+the dataset is written anyway. Check it with `vec validate` afterwards.
+
+| Problem | Strict (default) | `--no-strict` |
+| ------- | ---------------- | ------------- |
+| A required property has no value for some features | Error | Warning, the property is written as nullable |
+| An id repeats within a collection | Error | Warning |
+| The collection of a feature can't be determined | Error | Warning, the collection is left empty |
+| `-i` or `-e` removes a required property | Error | Warning |
+| A required collection-only property differs between the datasets | Error | Warning, the property is removed |
+| An optional collection-only property differs between the datasets | Warning, the property is removed | Warning, the property is removed |
+| A feature has an empty or missing geometry | Error | Warning, the feature is kept |
+| A collection-level value doesn't fit the data type of its schema | Error | Warning, the value is left empty |
+| A collection implements multiple versions of a schema, e.g. of an extension | Error | Error |
+| The schemas of the datasets conflict | Error | Error |
+
+The strict mode only checks what a merge can break or check with little effort.
+It doesn't validate the values against the schemas, e.g. patterns or value ranges,
+so a merged dataset is only valid if the values of the source datasets are valid.
+
 Check `vec merge --help` for more details.
 
 ### Create JSON Schema from Vecorel Schema

tests/test_convert_duckdb.py

@@ -1,4 +1,5 @@
 import json
+import re
 import sys
 
 import geopandas as gpd
@@ -9,7 +10,7 @@
 from loguru import logger
 
 from vecorel_cli.conversion.base import BaseConverter
-from vecorel_cli.conversion.duckdb import DuckDBBaseConverter
+from vecorel_cli.conversion.duckdb import DuckDBBaseConverter, _constants_table
 from vecorel_cli.validate import ValidateData
 from vecorel_cli.vecorel.hilbert import hilbert_keys_for_table
 
@@ -723,6 +724,25 @@ def test_merge_parquet_hydrates_nan_as_null(tmp_folder):
     }
 
 
+@pytest.mark.parametrize(
+    "value, dtype", [(300, "uint8"), ("not a date", "date-time")], ids=["overflow", "date-time"]
+)
+def test_constants_that_do_not_fit_their_type_are_reported(value, dtype, capsys):
+    log = Converter()
+    logger.remove()
+    logger.add(sys.stdout, format="{message}", level="DEBUG", colorize=False)
+    table = _constants_table({"x": value}, {"x": {"type": dtype}}, log=log, source="part.parquet")
+
+    # the value would fail the writer, so it's left empty
+    assert table.column("x").to_pylist() == [None]
+    message = f"x: Value {value!r} doesn't fit data type {dtype}"
+    out = capsys.readouterr().out
+    assert message in out and "(in part.parquet)" in out
+
+    with pytest.raises(ValueError, match=re.escape(message)):
+        _constants_table({"x": value}, {"x": {"type": dtype}}, log=log, strict=True)
+
+
 def test_merge_parquet_checks_ids_that_convert_generated(tmp_folder, capsys):
     """Each part numbered its own rows from zero, so the merge has to check what
     convert() is allowed to take for granted."""