Fractional seconds are lost

[rhauch]

I'm using 0.3.1, and think there is an issue with how the library is deserializing fractional seconds. In particular, the AbstractRowsEventDataDeserializer.getFractionalSeconds(...) method is defined as follows:

   private int getFractionalSeconds(int meta, ByteArrayInputStream inputStream) throws IOException {
        int fractionalSecondsStorageSize = getFractionalSecondsStorageSize(meta);
        if (fractionalSecondsStorageSize > 0) {
            long fractionalSeconds = bigEndianLong(inputStream.read(fractionalSecondsStorageSize), 0,
                    fractionalSecondsStorageSize);
            if (meta % 2 == 1) {
                fractionalSeconds /= 10;
            }
            return (int) (fractionalSeconds / 1000);
        }
        return 0;
    }

Here, meta is the metadata for the column, which for temporal types is the fractional seconds part, or fsp, as described in the MySQL documentation.

Consider an example table with this definition (taken from the MySQL documentation):

CREATE TABLE fractest (
  c1 TIME(2),
  c2 DATETIME(2),
  c3 TIMESTAMP(2)
);

and the following insert statement:

INSERT INTO fractest VALUES ('17:51:04.777', '2014-09-08 17:51:04.777', '2014-09-08 17:51:04.777');

Here, the fsp -- and thus the value of meta -- for all three columns is 2. The value of the fractionalSeconds variable read from the stream is then 78 (since rounding occurs within the database). However, the following lines in the method:

            if (meta % 2 == 1) {
                fractionalSeconds /= 10;
            }
            return (int) (fractionalSeconds / 1000);

seem to not make any sense, because the result will always be 0 unless fsp is 4, 5 or 6, and the result will always be incorrect.

If the function is to return the number of milliseconds, then it seems like the code should simply multiply by the correct factor of 10 (based on the number of digits in the precision, or meta):

            int factor = (3 - meta) * 10;
            return fractionalSeconds * factor;

So for fractionalSeconds=78 and meta=2, then factor=10 and the method return 780, which is the correct number of milliseconds.

For an fsp of 1, meta=1' andfractionalSeconds=8` (assuming MySQL rounded), then the method would return 800, which is the correct number of milliseconds.

Now, MySQL fsp values can be integers between 1 and 6, so the above code needs to amended to handle meta values that are greater than 3. A simple way to do that would be to change the factor to use exponentials, but a more efficient way is to do the following:

            if ( meta < 3 ) {
                int factor = (3 - meta) * 10;
                return fractionalSeconds * factor;
            }
            if ( meta > 3 ) {
                int divisor = (meta - 3) * 10;
                return (int) (fractionalSeconds / divisor);
            }
            return fractionalSeconds;

I'm sure there are other ways this could be written, but the above code uses simple integer math and is at least correct.

Note that this same problem appears in the master branch, too.

[rhauch]

Would you be willing to release a 0.3.2 with this fix? If not, when do you think you might release the library with this fix?

[rhauch]

Also, if these methods were not private I would be able to fix this (at least temporarily) in Debezium by subclassing and overriding the method. Any chance of converting methods like these to protected?

[shyiko]

Hi @rhauch

Would you be willing to release a 0.3.2 with this fix?

Of course, I'll do it right after work.

Also, if these methods were not private I would be able to fix this (at least temporarily) in Debezium by subclassing and overriding the method. Any chance of converting methods like these to protected?

All deserialize* are protected in master (though getFractionalSeconds is not). You can always register your own *RowsEventDataDeserializer if something is terribly wrong in the upstream but I usually try fixing critical stuff like this within 24 hours (provided it's clear where the problem is) so a ticket is all that is needed for a new release to happen.

Anyway, thank you for the bug report (and all the details)! I'll let you know when 0.3.2 is in Maven Central.

[rhauch]

@shyiko, sounds great. Thank you very much!

All deserialize* are protected in master (though getFractionalSeconds is not). You can always register your own *RowsEventDataDeserializer if something is terribly wrong in the upstream but I usually try fixing critical stuff like this within 24 hours (provided it's clear where the problem is) so a ticket is all that is needed for a new release to happen.

I actually looked at overriding, but because of the private method it would have required copying a lot of code. Making all the methods protected would be great for situations exactly like this, and would reduce the need for a patch release.

BTW, please double-check my logic, too.

[shyiko]

Making all the methods protected would be great for situations exactly like this, and would reduce the need for a patch release.

I understand. The reason I'm reluctant to marking all of them protected is that it's basically the same as public as far as backward-compatibility is concerned. Signatures have to stay the same, otherwise users may have to do a little bit more than just a simple version bump to upgrade. Anyhow, I agree that it could potentially help in situations like this and so I'll try to address this as part of 1.0.0.

BTW, please double-check my logic, too.

You can count on it :)

[shyiko]

0.3.2 deployed to Maven Central. Cheers 🍻