Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lost ArrayToStringDeserializer in ClickhouseResponse #851

Closed
debychkov opened this issue Mar 4, 2022 · 4 comments
Closed

Lost ArrayToStringDeserializer in ClickhouseResponse #851

debychkov opened this issue Mar 4, 2022 · 4 comments
Labels

Comments

@debychkov
Copy link
Contributor

debychkov commented Mar 4, 2022

Hi, i'm migrating from 0.2.6 to 0.3.2, and ran into another issue. Code example:

package ru.yandex.metrika.clickhouse;

import java.sql.SQLException;

import org.springframework.jdbc.datasource.DataSourceUtils;

import ru.yandex.clickhouse.ClickHouseConnection;
import ru.yandex.clickhouse.ClickHouseDataSource;
import ru.yandex.clickhouse.settings.ClickHouseProperties;

public class CreateDatabaseExample {

    public static void main(String[] args) throws SQLException {
        String connectionUrl = "jdbc:clickhouse://localhost:8123/default";
        ClickHouseConnection clickHouseConnection = (ClickHouseConnection) DataSourceUtils.getConnection(
                new ClickHouseDataSource(connectionUrl, new ClickHouseProperties()));
        clickHouseConnection.createStatement().executeQueryClickhouseResponse("SELECT 1 AS one, tuple(1,'hello', 'world') AS two");
    }
}

Fails with exception while parsing response:

Exception in thread "main" com.google.gson.JsonSyntaxException: java.lang.IllegalStateException: Expected a string but was BEGIN_ARRAY at line 16 column 8 path $.data[0][1]
	at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$Adapter.read(ReflectiveTypeAdapterFactory.java:226)
	at com.google.gson.Gson.fromJson(Gson.java:932)
	at com.google.gson.Gson.fromJson(Gson.java:870)
	at com.clickhouse.client.data.JsonStreamUtils.readObject(JsonStreamUtils.java:31)
	at com.clickhouse.client.data.JsonStreamUtils.readObject(JsonStreamUtils.java:27)
	at com.clickhouse.client.data.JsonStreamUtils.readObject(JsonStreamUtils.java:23)
	at ru.yandex.clickhouse.ClickHouseStatementImpl.executeQueryClickhouseResponse(ClickHouseStatementImpl.java:375)
	at ru.yandex.clickhouse.ClickHouseStatementImpl.executeQueryClickhouseResponse(ClickHouseStatementImpl.java:364)
	at ru.yandex.clickhouse.ClickHouseStatementImpl.executeQueryClickhouseResponse(ClickHouseStatementImpl.java:358)
	at ru.yandex.metrika.clickhouse.CreateDatabaseExample.main(CreateDatabaseExample.java:17)
Caused by: java.lang.IllegalStateException: Expected a string but was BEGIN_ARRAY at line 16 column 8 path $.data[0][1]
	at com.google.gson.stream.JsonReader.nextString(JsonReader.java:824)
	at com.google.gson.internal.bind.TypeAdapters$16.read(TypeAdapters.java:405)
	at com.google.gson.internal.bind.TypeAdapters$16.read(TypeAdapters.java:393)
	at com.google.gson.internal.bind.TypeAdapterRuntimeTypeWrapper.read(TypeAdapterRuntimeTypeWrapper.java:41)
	at com.google.gson.internal.bind.CollectionTypeAdapterFactory$Adapter.read(CollectionTypeAdapterFactory.java:82)
	at com.google.gson.internal.bind.CollectionTypeAdapterFactory$Adapter.read(CollectionTypeAdapterFactory.java:61)
	at com.google.gson.internal.bind.TypeAdapterRuntimeTypeWrapper.read(TypeAdapterRuntimeTypeWrapper.java:41)
	at com.google.gson.internal.bind.CollectionTypeAdapterFactory$Adapter.read(CollectionTypeAdapterFactory.java:82)
	at com.google.gson.internal.bind.CollectionTypeAdapterFactory$Adapter.read(CollectionTypeAdapterFactory.java:61)
	at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$1.read(ReflectiveTypeAdapterFactory.java:131)
	at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$Adapter.read(ReflectiveTypeAdapterFactory.java:222)
	... 9 more

It's probably related to #740.

ClickhouseResponse "data" node expects array of arrays (columns/rows), but when we have tuple in a result set it's also written as array, so Gson cannot parse the third array.

{
...
    "data":[
        [1, [1,'hello', 'world']]
     ]
...
}
@zhicwu
Copy link
Contributor

zhicwu commented Mar 5, 2022

Thanks for reporting the issue. It seems you're using the legacy driver. The new driver is com.clickhouse.jdbc.ClickHouseDriver.

Are you relying on JSON data format and extended API? If that's the case, you probably don't have to use JDBC driver but Java client.

@debychkov
Copy link
Contributor Author

Thanks for the answer.

We have a lot of interactions with clickhouse via jdbc in our code, and migrating it to http-client may take a lot of time. Also, yes, we are using queryJson a lot, so, turns out, we cannot migrate to non-legacy driver.

The scope of my task was just to update driver, so that it could parse CH exception codes with dots and commas alike.

I will try to make pull request, that preserves old behaviour, if it's ok. Gson does not seem to have nice annotations like ArrayToStringDeserializer, but custom deserializer should get the job done.

@debychkov
Copy link
Contributor Author

done: #862

I'm sorry, if anything wrong there - I'm a bit new to public github.

@zhicwu
Copy link
Contributor

zhicwu commented Mar 9, 2022

I see. Thanks @debychkov for the details and sorry for the inconvenience caused at your end.

We have a lot of interactions with clickhouse via jdbc in our code, and migrating it to http-client may take a lot of time.
Also, yes, we are using queryJson a lot, so, turns out, we cannot migrate to non-legacy driver.

Yes, it makes perfect sense to stay with JDBC API. It seems performance is not a concern, so it should be fine to use legacy driver. However, there are a few things you may want to consider:

  1. extended API / non-JDBC API
    There's always better driver, so it's better to avoid non-JDBC API. IMO, JSON de-serialization should be offloaded to application, so that you can choose favorite JSON lib and don't have to count on the driver. Most importantly, decoupling means you can later switch to a new driver without much cost.

  2. stability
    As you probably know, the legacy driver may raise failed to respond error. There were a few attempts trying to fix that but unfortunately it still exists. My guess is that it's related to how ClickHouse closes http connection and how Apache HttpClient handles that. On the other hand, it's proven on PROD and in other projects that the new driver does not have the stability issue - that's why it's highly recommended for the upgrade.

  3. support
    Very few changes being made to the legacy driver in 0.3.2, and perhaps less in 0.3.3. Apart from that, it will be removed starting from 0.4.0.

I'm sorry, if anything wrong there - I'm a bit new to public github.

Thanks for your contribution. I'll add some more tests along with additional changes in these days, and make it part of another patch in the coming weekend :)

@zhicwu zhicwu added bug module-jdbc JDBC driver labels Mar 9, 2022
@zhicwu zhicwu closed this as completed Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants