paul-rogers commented on PR #12872: URL: https://github.com/apache/druid/pull/12872#issuecomment-1272458763
Looked into the issues. There are several. Upshot: we cannot upgrade to this version: Avatica itself needs some fixes. We may have compatibility issues when older Avatica clients talk with newer Avatica servers. Version 1.22 appears to be a bit better at enforcing types. This has introduce multiple subtle failures in the tests, in part because Druid seems to be working around prior issues and those workarounds now seem to be causing problems. All of the issues are related to type propagation. Let's start with Protobuf. This protocol preserves the Java types we define in the server. Some tests fail because, before 1.22, the data sent across the wire would change type: `Double` would get deserialized as `Float`, say. With 1.22, tests have to be updated to expect `Double` values if we say that the column is of type `DOUBLE`. However, this uncovers test flaws. We expect fractional floating point values to compare exactly. They were equal when the type was converted to `Float`, but are no longer equal when the full `Double` type is preserved. Druid supports both the Prototbuf and JSON protocols. While Protobuf appears to preserve types, JSON does not: we're at the mercy of whatever Jackson chooses as the deserialized type. This is related to the fact that JSON has only a single number type: there is some fudging required to map to a Java type. The result is that we tell Avatica that the type is `BIGINT` (`Long`), but Jackson deserializes the value as `Integer` if it is small. This seems to work OK for scalar values, but in 1.22, it causes problems with arrays. Avatica expects that the elements in an array match the defined type. But, with JSON, we create an array of `Long` on the server, but Jackson deserializes them to an array of `Integer` on the client. Avatica attempts to cast the item to a `Long`, which fails. It would seem that Avatica should expect mixed `Integer` and `Long` values in this case, casting them to `Long` Druid appearently tried to work around this by declaring the Java type of some numbers to be `Number`. While this worked in 1.17, it does not work in 1.22. We have to declare the specific type. Thus, when doing this upgrade, we have four issues: * Avatica is broken. We probably have to submit a patch to get arrays to work with JSON. We can then consider using 1.23 (or later) with the patch. * Druid has to change to match the stricter type rules for 1.22. * Tests have to change to match the new behavior, and cannot expect that `Double` values compare exactly. * Compatibility With the last item, a worry is compatibility. Some of the changes do suggest that newer servers may not work with older clients. We cannot expect users to upgrade their clients concurrently with Druid: some cross-version interoperation is necessary. Thus, we need testing, which is a bit hard to do as a unit test or integration test. Net review is -1 on this change until we resolve the above issues. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
