feat(client-v2, jdbc-v2): add MultiPoint data type support - #3050
Open
polyglotAI-bot wants to merge 5 commits into
Open
feat(client-v2, jdbc-v2): add MultiPoint data type support#3050polyglotAI-bot wants to merge 5 commits into
polyglotAI-bot wants to merge 5 commits into
Conversation
MultiPoint is a geo type added in ClickHouse 26.8 and is Array(Point) on the wire, the same representation as Ring and LineString, so it is read and written as double[][]. The server also adds MultiPoint to the Geometry variant. It appends the new variant after the existing six instead of ordering it by type name, so the Geometry helper column now pins that order explicitly rather than relying on the generic Variant name ordering. MultiPoint is deliberately left out of the Geometry write-side mappings: it shares double[][] with Ring and LineString, so a value written to a Geometry column keeps resolving to Ring exactly as before. Implements: #3048
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
The Sonar quality gate reported 75% coverage on new code. The single uncovered spot was the two dimensional branch of DataTypeConverter.isGeoTypeForDimensions, where only one of the three type comparisons was exercised. Add a unit test that converts the same double[][] value through Variant(String, Ring), Variant(String, LineString) and Variant(String, MultiPoint), plus a contrast case with Variant(String, Polygon) that matches no two dimensional geo type and thus keeps the plain array form.
Collaborator
Author
|
CI triage after the first run:
|
Member
|
@cursor review |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds end-to-end support for ClickHouse 26.8+ MultiPoint geo type across the client-v2 and jdbc-v2 stacks, including correct Geometry variant discriminator handling (with MultiPoint appended last to preserve existing discriminator stability).
Changes:
- Introduces
ClickHouseDataType.MultiPointand wires it through column templates, binary read/write, dynamic geo tagging, SQL/parameter string rendering, and JDBC type mapping. - Updates
Geometryvariant construction to keep the server’s discriminator order stable while enabling decoding ofMultiPointstored insideGeometry. - Adds/extends unit and integration tests plus updates
docs/features.mdandCHANGELOG.mdfor the new user-visible behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| jdbc-v2/src/test/java/com/clickhouse/jdbc/metadata/DatabaseMetaDataTest.java | Extends type-literal expectations and nested-type expectations for server versions with/without MultiPoint. |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/JdbcDataTypeTests.java | Adds integration coverage for MultiPoint JDBC metadata, getObject/getArray, and insert paths. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/metadata/ResultSetMetaDataImpl.java | Treats MultiPoint like other geo types (“read as-is”) for class resolution. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/metadata/DatabaseMetaDataImpl.java | Adds MultiPoint literal prefix/suffix info ([ / ]) for type info rows. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java | Maps MultiPoint to JDBCType.ARRAY and to double[][].class in JDBC type/class mappings. |
| docs/features.md | Documents MultiPoint support and the Geometry discriminator ordering/ambiguity constraints. |
| client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java | Adds integration tests for concrete MultiPoint columns and decoding MultiPoint from Geometry. |
| client-v2/src/test/java/com/clickhouse/client/api/internal/DataTypeConverterTest.java | Adds coverage that 2D geo variants (incl. MultiPoint) render as point-sequence literals. |
| client-v2/src/test/java/com/clickhouse/client/api/data_formats/internal/SerializerUtilsTest.java | Adds MultiPoint custom dynamic-tag coverage and a byte-for-byte round-trip equivalence test vs Ring. |
| client-v2/src/main/java/com/clickhouse/client/api/internal/DataTypeConverter.java | Treats MultiPoint as a geo type and as a supported 2D geo literal rendering target. |
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java | Serializes MultiPoint via the existing geo ring array path and emits its custom dynamic type tag. |
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java | Reads MultiPoint using the existing readGeoRing() path. |
| clickhouse-data/src/test/java/com/clickhouse/data/ClickHouseColumnTest.java | Pins Geometry variant nested order (including MultiPoint last) and adds basic MultiPoint parsing/template tests. |
| clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseDataType.java | Adds the MultiPoint enum constant and its Java class mappings (double[][], ClickHouseGeoRingValue). |
| clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseColumn.java | Adds MultiPoint column template and constructs Geometry variant with explicit MultiPoint append. |
| CHANGELOG.md | Adds a user-facing entry describing MultiPoint support and the Geometry discriminator behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 58487f5. Configure here.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
Merged
7 tasks
…ype-support # Conflicts: # docs/features.md
Comment on lines
+247
to
+248
| case MultiPoint: | ||
| return (T) readGeoRing(); |
Collaborator
Author
There was a problem hiding this comment.
Thank you — the technical description of the Native layout is correct, and I verified it. But the behavior is pre-existing for every geo type and is not introduced by this PR, so I am not changing it here.
What this PR changes in this file: one line, case MultiPoint: return (T) readGeoRing();. NativeFormatReader is untouched by the diff.
Why MultiPoint is not special: NativeFormatReader.readBlock() enters its columnar Array branch only when column.isArray() is true. Point, Ring, LineString, MultiLineString, Polygon and MultiPolygon are all distinct ClickHouseDataType values, so isArray() is already false for each of them and each already falls through to binaryStreamReader.readValue(column). MultiPoint inherits exactly the same routing as its siblings.
Measured on main's code path (ClickHouse 26.7.3.19), using Ring — SELECT r FROM ... FORMAT Native for the rows [(1,2),(3,4)] and [(5,6)]:
010201720452696e67 1 col, 2 rows, name 'r', type Ring
02000000 00000000 03000000 00000000 cumulative offsets 2, 3
000000000000f03f 0000000000000840 0000000000001440 x column: 1, 3, 5
0000000000000040 0000000000001040 0000000000001840 y column: 2, 4, 6
Fed to NativeFormatReader:
row 0 = [[3.13151306251402E-294, 0.0], [1.7765824089018436E-307, 1.7835357647096786E-307]]
row 1 = desynchronized (NoSuchColumnException)
Expected [[1.0,2.0],[3.0,4.0]] then [[5.0,6.0]]. So Ring — with no code from this PR involved — already shows the failure mode you describe.
Why not fix it in this PR: either remedy (a Native columnar decode path for geo columns, or an explicit ClientException for geo in Native as readBlock already does for undecodable QBit shapes) changes the observable behavior of Point/Ring/LineString/MultiLineString/Polygon/MultiPolygon. That is a behavior change on existing types and needs its own PR and maintainer buy-in; bundling it into an additive MultiPoint feature would mix two concerns. Rejecting MultiPoint alone in Native would also make it inconsistent with the siblings that share its wire format.
The MultiPoint read/write tests in this PR run through RowBinaryWithNamesAndTypes, which is correct for all geo types today.
I have logged the pre-existing Native geo defect for a separate fix, including the reproduction above. It is distinct from #2955 / #2956 and from the Array element sub-structure work, because geo columns never enter the Array branch at all.
Leaving this thread open for maintainer visibility.
1 task
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b214923. Configure here.
| map.put(e.getKey(), double[].class); | ||
| break; | ||
| case LineString: | ||
| case MultiPoint: |
There was a problem hiding this comment.
Dead switch branch for MultiPoint in class map
Low Severity
The case MultiPoint: added inside the if (e.getValue().equals(JDBCType.OTHER)) branch of getDataTypeClassMap() is unreachable because CLICKHOUSE_TO_SQL_TYPE_MAP maps MultiPoint to JDBCType.ARRAY, not JDBCType.OTHER. The code suggests DATA_TYPE_CLASS_MAP maps MultiPoint to double[][].class, but at runtime the else branch runs instead and maps it to java.sql.Array.class. This is the same pre-existing pattern as the other geo types in this switch, but adding MultiPoint here extends the misleading dead code.
Reviewed by Cursor Bugbot for commit b214923. Configure here.
Collaborator
Author
There was a problem hiding this comment.
Thank you — the observation is factually correct, but it describes the switch as a whole, not something this PR introduces, so I am keeping the line. Details below.
Confirmed: CLICKHOUSE_TO_SQL_TYPE_MAP maps MultiPoint to JDBCType.ARRAY, so the if (e.getValue().equals(JDBCType.OTHER)) branch is not entered and case MultiPoint: does not execute. At runtime DATA_TYPE_CLASS_MAP.get(MultiPoint) is java.sql.Array.class, from the else branch. The PR test asserts exactly that observable result and passes in CI:
assertEquals(rsMd.getColumnClassName(geomColumn), Array.class.getName());This is the state of every geo type, not of MultiPoint. Point, Ring, LineString, Polygon, MultiLineString and MultiPolygon are all mapped to JDBCType.ARRAY as well, so all seven case labels in that switch are unreachable today. MultiPoint behaves identically to its siblings, which is the intended result for this feature.
Why removing only the MultiPoint label is worse than keeping it. The labels are dead only because of the current ARRAY mapping. If that mapping is ever changed for geo columns — the one change that revives this switch — then with the label present all three 2-dimensional geo types resolve to double[][].class together, while with the label removed MultiPoint alone falls through to default: and silently resolves to Object.class. Keeping the label makes the switch uniform and change-safe; dropping it plants a divergence that only appears later.
Cleaning up the dead geo labels is a reasonable change, but it applies to all seven types and belongs in its own PR against the pre-existing code, not in an additive MultiPoint feature. No behavior changes either way, so I am leaving this thread open for the maintainer to decide.
|
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.
Description
Implements #3048.
MultiPointis a geo type added in ClickHouse26.8. It isArray(Point)on the wire — the samerepresentation
RingandLineStringalready use — so the client reads and writes it asdouble[][]. Before this change the type was unknown to the client: reading or writing aMultiPointcolumn failed withUnknown data type: MultiPoint, and aMultiPointvalue inside aGeometrycolumn failed with an out-of-range variant discriminator.No public API is added; the change is a new
ClickHouseDataTypeconstant plus the type's handling onthe existing read, write, SQL-formatting, and JDBC metadata paths.
Design
Concrete
MultiPointcolumns followRing/LineStringat every site:double[][]/ClickHouseGeoRingValuevalue mapping,readGeoRingon the read path,GEO_RING_ARRAYserialization on the write path, the custom
Dynamictype tag, geo SQL rendering, and — injdbc-v2—Types.ARRAYwith type nameMultiPoint,double[][]fromgetObject, ajava.sql.ArrayfromgetArray, and[/]literal quoting ingetTypeInfo.Geometryneeds an explicit variant order. The server's discriminators (measured by reading thefirst
RowBinarybyte ofCAST(x AS Geometry)on26.8.1.1307) are:The first six are type-name-alphabetical, which is why the generic
Variantnested-column sort inClickHouseColumn.readColumnproduced the correct order for free.MultiPointbreaks thatcoincidence — the server appends it at 6 rather than inserting it name-ordered at 2, keeping the
existing discriminators stable.
createGeometryVariantColumn()therefore builds the columnexplicitly with
MultiPointlast instead of relying on the sort. The six pre-existing variants keeptheir positions, so
Geometrydecoding on25.11–26.7servers is unchanged.MultiPointis intentionally not writable throughGeometry. It shares its Java representation(
double[][]) withRingandLineString, so it cannot be selected by value shape. It is left outof both write-side maps (
classToVariantOrdNumMap,geometryTypeDimensionsToVariantOrdNumMap), whichmeans a 2D value written to a
Geometrycolumn keeps resolving toRingexactly as before — nobehavior change for existing users. Writing
MultiPointrequires a concreteMultiPointcolumn.This mirrors the pre-existing
Ring-vs-LineStringandPolygon-vs-MultiLineStringambiguityalready documented in
docs/features.md. For the same reasonMultiPointis read-only in aDynamiccolumn.Entry points covered:
client-v2(generic records, binary reader, POJO binding, insert path, SQLparameter formatting,
Dynamicread) andjdbc-v2(ResultSet,PreparedStatement,ResultSetMetaData,DatabaseMetaData).Changes
clickhouse-dataClickHouseDataType: newMultiPointconstant, and itsdouble[][]/ClickHouseGeoRingValueclass mapping.clickhouse-dataClickHouseColumn:MultiPointcolumn template;createGeometryVariantColumn()appendsMultiPointas the last variant while keeping the existingwrite-side mappings untouched.
client-v2BinaryStreamReader: readMultiPointviareadGeoRing.client-v2SerializerUtils: serializeMultiPointasGEO_RING_ARRAY; emit the custom geo typetag for
MultiPointin aDynamiccolumn.client-v2DataTypeConverter:MultiPointrenders as a geo literal and counts as a 2D geo type.jdbc-v2JdbcUtils/ResultSetMetaDataImpl/DatabaseMetaDataImpl:Types.ARRAYmapping,read-as-is handling, and
getTypeInfoliteral quoting.CHANGELOG.md,docs/features.md.Test
ClickHouseColumnTest.testGeometryVariantOrderpins the full nested order above, and asserts that a2D value and a
ClickHouseGeoRingValuestill resolve toRingon theGeometrywrite path.ClickHouseColumnTest.testMultiPointColumn: type parsing, column template,Array(MultiPoint).SerializerUtilsTest.testMultiPointRoundTrip: byte-for-byte identical output toRing, then readsthe value back.
testDynamicTypeTagUsesCustomEncodingForGeoTypesextended withMultiPoint.DataTypeTests.testMultiPoint(integration): aMultiPointcolumn in the middle of the schema witha trailing
Float64, written both through the POJO insert path and server-side viareadWKTMultiPoint, read back through generic records and the binary reader, asserting the values,the geo string rendering, and the trailing column.
DataTypeTests.testGeometryWithMultiPoint(integration): reads aMultiPointout of aGeometrycolumn (discriminator 6) with a
Ringrow as the contrast case, pinning thatRingdecoding isunchanged.
JdbcDataTypeTests.testGeoMultiPoint(integration): metadata (getColumnType,getColumnTypeName,getColumnClassName),getObject,getArray, insert throughcreateArrayOf("Array(Point)", ...)and throughreadWKTMultiPoint.(,26.7].Verified green on both ClickHouse
26.7.3.19and26.8.1.1307:clickhouse-data(1667 tests),client-v2andjdbc-v2unit tests, andDataTypeTests,RowBinaryFormatWriterTest,JdbcDataTypeTests,DatabaseMetaDataTestintegration suites.Existing tests touched
Three existing tests needed the new enum constant registered; none were weakened:
DataTypeTests— the two exhaustiveClickHouseDataType.values()loops that buildVariant(String, <type>)tables skipMultiPointfor the same reason they already skipLineStringandMultiLineString(identical Java representation, tested separately).DatabaseMetaDataTest.TYPE_LITERAL_EXPECTATIONS— addedMultiPointwith the[/]literals usedby every other array-like geo type; without it the test would assert null literals.
DatabaseMetaDataTest.testFindNestedTypes—MultiPointis absent fromsystem.data_type_familiesbefore26.8, so it is expected to be missing there. The assertion isstill an exact comparison.
Docs / surface
CHANGELOG.mdentry added, with the issue link.docs/features.mdupdated in all four affected places: theclient-v2andjdbc-v2featurebullets (new
MultiPointsupport plus the extendedGeometryvariant), the twocompatibility-sensitive notes on
Geometrywrite inference, and thejdbc-v2custom type-mapbullet that enumerates the geometry types bypassing the map.
VERSIONis already the unreleased0.11.0-rc1this entry belongs to.docs/changes_checklist.md— "Enum constant added"MultiPointisinserted into the geo block (after
MultiLineString) rather than appended at the end of the enum.Ordinals are not persisted or serialized anywhere: the binary type tag is the explicit
binTagfield, which is
-1for every geo type. Appending at the very end would in fact have been theriskier choice —
binTag2Typeis keyed bybinTag, so all-1types collide and the lastconstant wins; inserting in the geo block leaves that entry unchanged. This also follows the
placement precedent set by
LineStringandMultiLineString.switch/map sites inclickhouse-data,client-v2, andjdbc-v2were updated. The v1 stack (clickhouse-jdbcJdbcTypeMapping,clickhouse-dataClickHouseRowBinaryProcessor) enumerates onlyPoint/Ring/Polygon/MultiPolygonand already predatesLineString,MultiLineString, andGeometry; it is out of scope for aclient-v2/jdbc-v2feature.and the three integration tests above.
Pre-PR validation gate
ResultSet/PreparedStatement)MultiPointfollowsRing/LineStringat every siteclient-v2+jdbc-v2, concrete column +Geometry+Dynamicread)RingpathCHANGELOG.md+docs/features.mdupdatedAGENTS.md,docs/changes_checklist.md,docs/features.md