Keep M measures when writing to WKB and reading from WKB

[kaiwinter]

The changes I made (adding ordinateFlags) are mainly taken over from the WKTReader/WKTWriter.

[kaiwinter]

Are there still open issues or may this PR be merged?

[oers]

Any status on this?

[oers]

I had to fix this in WKBWriter, to make it work with XYZM

    private void writeCoordinate(CoordinateSequence seq, int index, OutStream os)
            throws IOException
    {
        ByteOrderValues.putDouble(seq.getX(index), buf, byteOrder);
        os.write(buf, 8);
        ByteOrderValues.putDouble(seq.getY(index), buf, byteOrder);
        os.write(buf, 8);

        // only write 3rd dim if caller has requested it for this writer
        if (outputDimension >= 3) {
            // if 3rd dim is requested, only write it if the CoordinateSequence provides it
            double ordVal = Coordinate.NULL_ORDINATE;
            if (seq.getDimension() == 3)
                ordVal = seq.getOrdinate(index, 2);
            ByteOrderValues.putDouble(ordVal, buf, byteOrder);
            os.write(buf, 8);
        }
        // only write 4th dim if caller has requested it for this writer
        if (outputDimension == 4) {
            // if 4th dim is requested, only write it if the CoordinateSequence provides it
            double ordVal = Coordinate.NULL_ORDINATE;
            if (seq.getDimension() == 4)
                ordVal = seq.getOrdinate(index, 3);
            ByteOrderValues.putDouble(ordVal, buf, byteOrder);
            os.write(buf, 8);
        }
    }

Test for XYZM is missing here

[kaiwinter]

@oers Does the WKTWriter handle XYZM coordination correctly? If not this should better be fixed together in a new pull request maybe. If it does I could add your fix to this pull request.

[oers]

@kaiwinter yes, the textwriter works correctly. I tested both with XYZM Coordinates (reading and writing)

[kaiwinter]

@oers I don't understand the implementation tbh. I tried to adapt an existing unit test but couldn't even get the WKTReader to parse a XYZM coordinate. So I couldn't verify what your change does exactly. Maybe this should better be an own issue and pull request? What does @dr-jts think?

[oers]

The wrong method in WKB Writer never wrote the 4th dimension. My Method fixes this (the new one below. It still had a bug)

    private void writeCoordinate(CoordinateSequence seq, int index, OutStream os)
            throws IOException
    {
        ByteOrderValues.putDouble(seq.getX(index), buf, byteOrder);
        os.write(buf, 8);
        ByteOrderValues.putDouble(seq.getY(index), buf, byteOrder);
        os.write(buf, 8);

        // only write 3rd dim if caller has requested it for this writer
        if (outputDimension >= 3) {
            // if 3rd dim is requested, only write it if the CoordinateSequence provides it
            double ordVal = seq.getOrdinate(index, 2);
            ByteOrderValues.putDouble(ordVal, buf, byteOrder);
            os.write(buf, 8);
        }
        // only write 4th dim if caller has requested it for this writer
        if (outputDimension == 4) {
            // if 4th dim is requested, only write it if the CoordinateSequence provides it
            double ordVal = seq.getOrdinate(index, 3);
            ByteOrderValues.putDouble(ordVal, buf, byteOrder);
            os.write(buf, 8);
        }
    }

My Test is like this:

    @Test
    void wkbLineStringZM() throws ParseException {
        LineString lineZM = fac.createLineString(new Coordinate[]{new CoordinateXYZM(1,2,3,4), new CoordinateXYZM(5,6,7,8)});
        byte[] write = new WKBWriter(4).write(lineZM);


        LineString deserialisiert = (LineString) new PostgisWKBReader().read(write);

        assertEquals(lineZM, deserialisiert);

        for (int i = 0; i < lineZM.getNumPoints() ; i++) {
            Point geometryN = lineZM.getPointN(1);
            Point geometryN1 = deserialisiert.getPointN(1);

            assertFalse(Double.isNaN(geometryN.getCoordinate().getM()));
            assertFalse(Double.isNaN(geometryN.getCoordinate().getZ()));
            assertFalse(Double.isNaN(geometryN1.getCoordinate().getM()), "M is persisted");
            assertFalse(Double.isNaN(geometryN1.getCoordinate().getZ()), "Z is persisted");

            assertEquals(geometryN, geometryN1);
        }
    }

    @Test
    void wktLineStringZM() throws ParseException {
        LineString lineZM = fac.createLineString(new Coordinate[]{new CoordinateXYZM(1,2,3,4), new CoordinateXYZM(5,6,7,8)});
        String write = new WKTWriter(4).write(lineZM);

        LineString deserialisiert = (LineString) new PostgisWKTReader().read(write);

        assertEquals(lineZM, deserialisiert);

        for (int i = 0; i < lineZM.getNumPoints() ; i++) {
            Point geometryN = lineZM.getPointN(1);
            Point geometryN1 = deserialisiert.getPointN(1);

            assertFalse(Double.isNaN(geometryN.getCoordinate().getM()));
            assertFalse(Double.isNaN(geometryN.getCoordinate().getZ()));
            assertFalse(Double.isNaN(geometryN1.getCoordinate().getM()), "M is persisted");
            assertFalse(Double.isNaN(geometryN1.getCoordinate().getZ()), "Z is persisted");

            assertEquals(geometryN, geometryN1);
        }
    }

[kaiwinter]

@oers I added your changes to this pull request.

[oers]

I integrated the changes made here in a project that uses postgis 14. Itworks really well.

[jiayuasu]

@kaiwinter @dr-jts Thanks for the great work. Is there a reason why this PR hasn't been merged after a year? Is there anything others could help?

WKB format is often used for geometry serialization. Silently missing the M value will lead to incorrect results. We at Sedona uses WKB to serialize XYZM data for our Snowflake support.

[jodygarnett]

Can you rebase and resolve conflicts please?

[jiayuasu]

@jodygarnett I am not the author of this PR. @kaiwinter could you help us rebase and resolve conflicts?

I will wait for @kaiwinter 2-3 days. If he is busy, I will be happy to take over the codebase and create a new PR.

This issue might affect multiple Java-based DB engines if Parquet's Geometry type gets merged. We would really appreciate your help to accelerate the review of this PR when the new PR / rebase is ready..

[kaiwinter]

I did a rebase. Would be great if the PR finally gets merged.

[jnh5y]

I kicked off CI just now; let's see if it builds a-ok!

[jodygarnett]

Reading through this I only had minor feedback.

I expect that this may break the GeoTools subclass of WKBReader / WKBWriter that handles curves. With that in mind I am going to check with the GeoTools community to see if some of these methods need to be changed to protected (but this can be done in a separate PR).

We have two things of feedback that can be done as distinct PRs to avoid holding this fix up:

• The use of EnumSet is a nit more complicated then just covering XY, XYM, XYZ, XYZM cases. If desired such a change should happen in several sections of code.
• WKBWriter / WKBReader have several private methods that GeoTools subclass duplicate since they cannot override.

Is that about it?

[jodygarnett]

I am now testing a local build of this PR for any regression in GeoTools stack.

[jnh5y]

I am now testing a local build of this PR for any regression in GeoTools stack.

@jodygarnett cool. Can you make any of your branches public? It'd be awesome to see if @elahrvivaz can test GeoMesa with those branches.

Also, @jiayuasu is there any testing that the Sedona / GeoParquet communities can be doing here?

Generally, I'd like to point out that we have plenty of time to line all this up! Jody started the paperwork with Eclipse which needs to be done before we release. That'll take a few weeks, so we can take our time, etc.

[jodygarnett]

I am taking notes here geotools/geotools#4884

The branch for that PR is https://github.com/jodygarnett/geotools/tree/jts_1.20.0_update

[jodygarnett]

Oh I kind of screwed up, this PR should have included a change to the release notes. Something to consider for next time.