Add MultiPoint data type - #109951
Conversation
`MultiPoint` is stored as `Array(Point)` and added to the `Geometry` Variant. Geo functions do not support it yet and throw `NOT_IMPLEMENTED`. Since `Variant` sorts alternatives by name, the `Geometry` discriminators are renumbered, which affects backwards compatibility. Resolves ClickHouse#109852
|
@alexey-milovidov Let me know how this looks, two things:
|
Let's add all in this PR. |
|
Workflow [PR], commit [d31235b] Summary: ✅
AI ReviewSummaryThis PR adds the Findings
Final VerdictStatus: |
|
@alexey-milovidov Everything should be implemented now. For backward compatibility MultiPoint is appended to the end of the Geometry variant |
…sentable The `MultiPoint` type is now a first-class `Geometry` type, so `GeoJSON` reading stores it in the `MultiPoint` variant and only `GeometryCollection` stays unrepresentable. The hand-maintained `docs/en/interfaces/formats/GeoJSON.md` already reflected this, but the in-code structured documentation in `GeoJSONRowInputFormat.cpp` (and the regenerated `docs/reference/formats/GeoJSON.mdx`) still described `MultiPoint` as unrepresentable. Update both so the docs match the behavior tested by `04003_geojson_input_format`, `04490_geojson_input_geometry_collection`, and `04510_geojson_multipoint`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…erical} `checkGeometryIntersectArgument` did not allow the named `MultiPoint` type, so a concrete `MultiPoint` argument threw `ILLEGAL_TYPE_OF_ARGUMENT`, and through the `Geometry` type the Variant adaptor treated the `MultiPoint` alternative as incompatible and silently inserted the default result for those rows. Allow `MultiPoint` (the dispatch in `callOnGeometryDataType` distinguishes it by the custom type name), mention it in the ambiguity error for the unnamed `Array(Tuple(Float64, Float64))` form, update the docs, and add regression queries for both concrete `MultiPoint` and `MultiPoint` inside `Geometry`.
…ql_geometry CI: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109951&sha=bb3d9e1b5ac15619831441c318769e1104f93074&name_0=PR `test_mysql_geometry` failed because `parseWKBFormat` now parses `MULTIPOINT` WKB (this PR made it representable), but `insertGeometryValue` in `MySQLSource.cpp` had no `MultiPoint` branch, so the concrete type name stayed empty and reading a generic `geometry` column holding a MULTIPOINT threw `Cannot store a geometry of type into a Geometry column`. - Serialize the parsed `MultiPoint` in `insertGeometryValue`. - Map MySQL's concrete `multipoint` column type to `MultiPoint` in `convertMySQLDataType` (previously it fell back to `String` because there was no counterpart). - Update `test_mysql_geometry`: MULTIPOINT in a generic `geometry` column now reads back as a value; the unrepresentable-subtype regression uses `GEOMETRYCOLLECTION` instead. - Update the `mysql_datatypes_support_level` setting doc and the MySQL database engine doc: only `GEOMETRYCOLLECTION` has no ClickHouse counterpart now.
|
Pushed fixes for the CI reds of commit bb3d9e1:
Unrelated failures:
|
`convertMySQLDataType` maps MySQL `MULTIPOINT` columns to `MultiPoint`, but `ExternalResultDescription::init` did not route `DataTypeMultiPointName` through the geometry branch, so such columns fell through to `vtArray` and `MySQLSource::insertValue` threw `Unsupported value type` when reading them (`test_mysql_geometry` failure on 5 integration jobs). CI: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109951&sha=f4b5ef3798509611f5a9aac5056bb242f7432013&name_0=PR PR: ClickHouse#109951 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed d880181 fixing the The remaining two reds on f4b5ef3 are unrelated to this PR:
|
|
@alexey-milovidov this is STID 4752-5ad8, the chronic The fix is already in progress: #110717 (open, under review). It adds a |
The `mysql_datatypes_support_level` entry in `SettingsChangesHistory.cpp` and the `session-settings.mdx` mirror still described `MULTIPOINT` as having no ClickHouse counterpart, although this PR maps MySQL `MULTIPOINT` to `MultiPoint`. Also regenerate the `flipCoordinates` section of `other-functions.mdx` from the updated `FunctionDocumentation`, which now includes `MultiPoint`. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109951&sha=01b0e292205890653dd65826189752f3c8cc74cc&name_0=PR ClickHouse#109951 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Status update on
|
|
Acknowledged. Searched open/closed PRs and issues: no in-progress fix for |
|
Investigated Evidence (30d CIDB,
q95 is blameless: it already spills GROUP BY and JOIN to disk at 300 MB ( The mitigations for this shard are already merged:
Per the tuning log now in Conclusion: no q95 PR. The remaining knob is this job's memory ratio vs the host-OOM invariant your |
# Conflicts: # docs/reference/settings/formats.mdx # docs/reference/settings/session-settings.mdx
Master split the autogenerated `docs/reference/settings/formats.mdx` and `session-settings.mdx` into per-group pages (ClickHouse#111434). The merge of master resolved the conflict by taking master's split pages, which were generated without this branch's changes, dropping three previously-reviewed doc fixes. Port them into the new split pages: - `input-format.mdx`: `input_format_geojson_unsupported_geometry_handling` no longer lists `MultiPoint` as unrepresentable (matches `FormatFactorySettings.h` and `SettingsChangesHistory.cpp`), and `input_format_parquet_allow_geoparquet_parser` mentions `MultiPoint`. - `session-settings/mysql.mdx`: `mysql_datatypes_support_level` maps MySQL `MULTIPOINT` to `MultiPoint`; only `GEOMETRYCOLLECTION` remains unrepresentable (matches `Settings.cpp`). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Status update: @davidmenggx merged current master into the branch (
Also: the failed-job rerun on |
|
CI status on
Merged current master ( The AI-review verdict still shows "Request changes", but all three Majors are marked dismissed by author: they concern the pre-existing structural ambiguity of the geo family ( |
`strAppendGeoValue` picked the concrete geo type for a `Geometry` value from `1..GeoTypes::MultiPolygon`, but the new `GeoTypes::MultiPoint = 8` enumerator sits after `GeoTypes::Geometry = 7` in the proto enumeration, so the fuzzer could never produce a `MultiPoint` alternative inside `Geometry`. Remap a draw of `Geometry` to `MultiPoint` so all seven concrete alternatives are generated uniformly. Addresses the AI review finding on this line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Update (head
Remaining reds on
The three dismissed threads ( |
|
Acknowledged. Confirmed via CIDB this is a chronic cross-PR/trunk crash, not caused by this PR: |
|
Follow-up: the fix is in #111873. There was no core to pull: the job never wrote one. The only trace of the crash is the watchdog line
With no stack, no core, and in-flight queries unrelated to this PR, the SIGSEGV itself cannot be located, and I am not speculating an engine fix without evidence. #111873 fixes all three diagnostics gaps so the next occurrence of this chronic crash (27 master + ~25 unrelated-PR hits in 30 days) carries the signal name and a collectable core dump. |
`QueryFuzzer::getRandomType` enumerated only the six pre-existing geo type aliases, so this long-running fuzzing path could not generate `MultiPoint` columns or casts. Add `MultiPoint` to the list, matching the `BuzzHouse` generator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
CI on 58b3169 completed: 154 passed, one red — Upgrade check (amd_release), unrelated to this PR:
The AI review verdict flipped to ✅ Approve; the single remaining Nit ( |
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 601/688 (87.35%) · Uncovered code |
Master removed ci/tests/test_log_parser.py in 3a69545, which deleted 71 ci/tests files and added to .claude/CLAUDE.md: "you don't need to commit it in the repository, because we don't have automated CI tests". 39 of those files were selectively restored afterwards; this one was not. Taking master's side. The parser and shell changes are unaffected: master's Signal pattern still matches only signal 9, and stress_tests.lib still lacks both the per-start core_path config and the core-limit evidence line. Related: ClickHouse#109951 Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109951&sha=5ec2b2ed5b6cf1fc8ede04ce6756bebc1e281cda&name_0=PR&name_1=Stress%20test%20%28arm_asan_ubsan%2C%20s3%29
Added the
MultiPointgeo data type, stored asArray(Point), and included it in theGeometrytype. Geo functions (readWKT/readWKB,wkt,wkb,svg,area,perimeter,polygons*, MVT encoding, GeoJSON input/output, Arrow/Parquet geo metadata) support it.MySQL integration (the
mysqltable function/engine and the MySQL database engine) maps the concreteMULTIPOINTcolumn type toMultiPoint, and a genericGEOMETRYcolumn holding aMULTIPOINTvalue reads back through theGeometrytype'sMultiPointalternative (previously both threw or fell back toString; onlyGEOMETRYCOLLECTIONremains unrepresentable).To keep backward compatibility,
Geometrynow builds itsVariantwith a fixed discriminator order (matching the previous sorted-by-name order) andMultiPointis appended at the end, so discriminators persisted in existing data keep their meaning. This is covered by the test04511_geometry_discriminators_compatibility, which reads hardcodedNativeandRowBinaryblobs produced by a pre-MultiPointserver.Closes: #109852
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Added the
MultiPointgeo data type, stored asArray(Point), and included it in theGeometrytype.Version info
26.8.1.146(included in26.8and later)