diff --git a/CHANGELOG.md b/CHANGELOG.md index 69cd64124..1e558e56a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -171,6 +171,14 @@ ### Bug Fixes +- **[client-v2]** Fixed geo columns (`Point`, `Ring`, `LineString`, `MultiPoint`, `Polygon`, `MultiLineString`, + `MultiPolygon`) being misread from the `Native` format. The reader decoded a geo column row by row with the + RowBinary decoders, while `Native` writes it column-major: a `Point` block came back with its coordinates + scrambled across rows and no error, and every other geo type desynchronized the block and failed with + `Non-empty typeName is required`. A geo column is now decoded from the Native layout - the two `Float64` + sub-columns of a point, and cumulative offsets plus the flattened elements for the array levels - and returns + the same values as `RowBinaryWithNamesAndTypes`, which is unchanged. + (https://github.com/ClickHouse/clickhouse-java/issues/3088) - **[client-v2, jdbc-v2]** Fixed a column type with a `JSON` element that is followed by a parameterized type, for example `Tuple(JSON, FixedString(3))`, being parsed wrongly. `JSON` is valid with and without a parameter list, and the parser looked for the opening bracket of that list anywhere after the keyword, so it took the brackets of the diff --git a/client-v2/src/main/java/com/clickhouse/client/api/data_formats/NativeFormatReader.java b/client-v2/src/main/java/com/clickhouse/client/api/data_formats/NativeFormatReader.java index a78f1a45e..0f17e419b 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/data_formats/NativeFormatReader.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/data_formats/NativeFormatReader.java @@ -106,6 +106,12 @@ private boolean readBlock() throws IOException { + "strided, wrapped in Nullable/LowCardinality, or nested inside another type " + "(e.g. Array/Tuple/Map), is not decoded. Use a RowBinary format " + "(e.g. RowBinaryWithNamesAndTypes) to read such QBit values"); + } else if (isGeo(column)) { + // Native writes a geo column column-major: a Point as the two Float64 sub-columns of its + // tuple, and every other geo type as cumulative offsets followed by the flattened elements + // of the level below. The per-row decoders reached through readValue() read the RowBinary + // layout instead, which scrambles a Point block and desynchronizes the columns that follow. + values = binaryStreamReader.readGeoNative(column, nRows); } else if (column.isArray()) { // Native encodes an Array column as nRows cumulative offsets followed by the // flattened elements; each row's element count is the delta between consecutive @@ -164,6 +170,28 @@ private static boolean isNativeDecodableQBit(ClickHouseColumn column) { } } + /** + * Returns {@code true} for a geo column ({@code Point}, {@code Ring}, {@code LineString}, + * {@code MultiPoint}, {@code Polygon}, {@code MultiLineString} or {@code MultiPolygon}). Geo types + * are their own data types rather than {@code Array}/{@code Tuple}, so they do not reach the + * columnar branches of {@link #readBlock} on their own and need their own Native decoder + * ({@link BinaryStreamReader#readGeoNative(ClickHouseColumn, int)}). + */ + private static boolean isGeo(ClickHouseColumn column) { + switch (column.getDataType()) { + case Point: + case Ring: + case LineString: + case MultiPoint: + case Polygon: + case MultiLineString: + case MultiPolygon: + return true; + default: + return false; + } + } + /** * Returns {@code true} if {@code column} is or contains a {@code QBit} anywhere in its nested type * tree (e.g. {@code Array}/{@code Tuple}/{@code Map(String, QBit(...))}). Used by {@link #readBlock} diff --git a/client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java b/client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java index dbdbd559b..de4504856 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java @@ -30,6 +30,7 @@ import java.time.ZonedDateTime; import java.time.temporal.TemporalAmount; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; @@ -1176,6 +1177,112 @@ private double[][][][] readGeoMultiPolygon() throws IOException { return value; } + /** + * Reads a whole geo column of a Native block. Native is columnar: a {@code Point} + * (= {@code Tuple(Float64, Float64)}) is written as all X coordinates followed by all Y + * coordinates, and every other geo type is an array whose level is written as cumulative + * {@code UInt64} offsets followed by the flattened elements of the next level. The row-wise + * decoders ({@link #readGeoPoint()} and friends) read the RowBinary layout instead and cannot + * be used for a Native block. + * + * @param column geo column + * @param nRows number of rows in the block + * @return one value per row, in the same shape the RowBinary decoders return: {@code double[]} + * for a point, {@code double[][]} for a ring, {@code double[][][]} for a polygon and + * {@code double[][][][]} for a multipolygon + * @throws IOException when IO error occurs + */ + public List readGeoNative(ClickHouseColumn column, int nRows) throws IOException { + Object[] values; + switch (column.getDataType()) { + case Point: + values = readGeoPointsNative(nRows); + break; + case Ring: + case LineString: + case MultiPoint: + values = readGeoRingsNative(nRows); + break; + case Polygon: + case MultiLineString: + values = readGeoPolygonsNative(nRows); + break; + case MultiPolygon: + values = readGeoMultiPolygonsNative(nRows); + break; + default: + throw new ClientException("Not a geo column: " + column.getOriginalTypeName()); + } + + List result = new ArrayList<>(nRows); + Collections.addAll(result, values); + return result; + } + + /** + * Reads {@code nPoints} points of a Native geo column: all X coordinates, then all Y coordinates. + */ + private double[][] readGeoPointsNative(int nPoints) throws IOException { + double[][] points = new double[nPoints][2]; + for (int i = 0; i < nPoints; i++) { + points[i][0] = readDoubleLE(); + } + for (int i = 0; i < nPoints; i++) { + points[i][1] = readDoubleLE(); + } + return points; + } + + private double[][][] readGeoRingsNative(int nRings) throws IOException { + return groupByGeoOffsetsNative(this::readGeoPointsNative, nRings); + } + + private double[][][][] readGeoPolygonsNative(int nPolygons) throws IOException { + return groupByGeoOffsetsNative(this::readGeoRingsNative, nPolygons); + } + + private double[][][][][] readGeoMultiPolygonsNative(int nMultiPolygons) throws IOException { + return groupByGeoOffsetsNative(this::readGeoPolygonsNative, nMultiPolygons); + } + + /** + * Reads one array level of a Native geo column: {@code nGroups} cumulative {@code UInt64} offsets, + * then the flattened elements of the level below, which are then split back into groups. + */ + private T[][] groupByGeoOffsetsNative(GeoLevelReader elementReader, int nGroups) throws IOException { + long[] offsets = new long[nGroups]; + long prevOffset = 0; + for (int i = 0; i < nGroups; i++) { + long offset = readLongLE(); + if (offset < prevOffset || offset > Integer.MAX_VALUE) { + throw new ClientException("Invalid offset in a Native geo column: expected a non-decreasing" + + " offset not greater than " + Integer.MAX_VALUE + ", got " + offset + " after " + + prevOffset); + } + offsets[i] = offset; + prevOffset = offset; + } + + T[] elements = elementReader.read(nGroups == 0 ? 0 : Math.toIntExact(offsets[nGroups - 1])); + return splitByGeoOffsetsNative(elements, offsets); + } + + @SuppressWarnings("unchecked") + private static T[][] splitByGeoOffsetsNative(T[] elements, long[] offsets) { + T[][] groups = (T[][]) Array.newInstance(elements.getClass(), offsets.length); + int prevOffset = 0; + for (int i = 0; i < offsets.length; i++) { + int offset = Math.toIntExact(offsets[i]); + groups[i] = Arrays.copyOfRange(elements, prevOffset, offset); + prevOffset = offset; + } + return groups; + } + + private interface GeoLevelReader { + T[] read(int count) throws IOException; + } + /** * Reads a varint from input stream. * diff --git a/client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java b/client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java index cedf9f263..70ba11b23 100644 --- a/client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java +++ b/client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java @@ -51,6 +51,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -2338,6 +2339,86 @@ public void testGeometryWithMultiPoint() throws Exception { Assert.assertEquals(records.get(1).getDouble("marker"), 42D); } + @DataProvider(name = "geoTypes") + public static Object[][] geoTypes() { + final String ring = "[(toFloat64(number * 10 + 1), toFloat64(number * 10 + 2))," + + " (toFloat64(number * 10 + 3), toFloat64(number * 10 + 4))]"; + return new Object[][] { + {"Point", ClickHouseDataType.Point, + "(toFloat64(number * 2 + 1), toFloat64(number * 2 + 2))::Point", + new Object[] {pointOf(0), pointOf(1), pointOf(2)}, null}, + {"Ring", ClickHouseDataType.Ring, ring + "::Ring", + new Object[] {ringOf(0), ringOf(1), ringOf(2)}, null}, + {"LineString", ClickHouseDataType.LineString, ring + "::LineString", + new Object[] {ringOf(0), ringOf(1), ringOf(2)}, null}, + {"MultiPoint", ClickHouseDataType.MultiPoint, ring + "::MultiPoint", + new Object[] {ringOf(0), ringOf(1), ringOf(2)}, MULTI_POINT_UNSUPPORTED_VERSIONS}, + {"Polygon", ClickHouseDataType.Polygon, "[" + ring + "]::Polygon", + new Object[] {polygonOf(0), polygonOf(1), polygonOf(2)}, null}, + {"MultiLineString", ClickHouseDataType.MultiLineString, "[" + ring + "]::MultiLineString", + new Object[] {polygonOf(0), polygonOf(1), polygonOf(2)}, null}, + {"MultiPolygon", ClickHouseDataType.MultiPolygon, "[[" + ring + "]]::MultiPolygon", + new Object[] {multiPolygonOf(0), multiPolygonOf(1), multiPolygonOf(2)}, null}, + // The CAST wraps the whole if(): the common type of the two branches is + // Array(Tuple(Float64, Float64)) on some server versions, so only an outer CAST makes + // the column a Ring everywhere. + {"Ring, empty value", ClickHouseDataType.Ring, + "CAST(if(number = 1, [], " + ring + ") AS Ring)", + new Object[] {ringOf(0), new double[0][], ringOf(2)}, null}, + }; + } + + private static double[] pointOf(int row) { + return new double[] {row * 2 + 1D, row * 2 + 2D}; + } + + private static double[][] ringOf(int row) { + return new double[][] {{row * 10 + 1D, row * 10 + 2D}, {row * 10 + 3D, row * 10 + 4D}}; + } + + private static double[][][] polygonOf(int row) { + return new double[][][] {ringOf(row)}; + } + + private static double[][][][] multiPolygonOf(int row) { + return new double[][][][] {polygonOf(row)}; + } + + @Test(groups = {"integration"}, dataProvider = "geoTypes") + public void testGeoTypesReadInEveryBinaryFormat(String typeName, ClickHouseDataType expectedType, + String expression, Object[] expected, String unsupportedVersions) throws Exception { + if (unsupportedVersions != null && isVersionMatch(unsupportedVersions)) { + return; + } + + // The geo column sits between two fixed-width columns and the block holds several rows, so a + // column read with the wrong layout shows up as wrong values and as a desynchronized marker. + String sql = "SELECT toInt32(number) AS rowId, " + expression + " AS geom, toInt32(42) AS marker" + + " FROM numbers(" + expected.length + ") ORDER BY rowId"; + for (ClickHouseFormat format : new ClickHouseFormat[] {ClickHouseFormat.Native, + ClickHouseFormat.RowBinaryWithNamesAndTypes}) { + String label = typeName + " in " + format; + try (QueryResponse response = client.query(sql, new QuerySettings().setFormat(format)).get()) { + ClickHouseBinaryFormatReader reader = client.newBinaryFormatReader(response); + // The server decides the type of the expression, so pin it: a version that returns + // Array(Tuple(Float64, Float64)) here would read a different code path and leave the + // geo one untested. + Assert.assertEquals(reader.getSchema().getColumnByName("geom").getDataType(), expectedType, + label); + int rows = 0; + while (reader.next() != null) { + Object value = reader.readValue("geom"); + Assert.assertEquals(reader.getInteger("rowId"), rows, label); + Assert.assertTrue(Objects.deepEquals(value, expected[rows]), + label + " row " + rows + ": " + Arrays.deepToString(new Object[] {value})); + Assert.assertEquals(reader.getInteger("marker"), 42, label); + rows++; + } + Assert.assertEquals(rows, expected.length, label); + } + } + } + @Test(groups = {"integration"}) public void testDates() throws Exception { LocalDate date = LocalDate.of(2024, 1, 15);