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

JDBC Driver marks Nullable Column as non-nullable #1119

Open
nym3r0s opened this issue Nov 8, 2022 · 12 comments · Fixed by #1124
Open

JDBC Driver marks Nullable Column as non-nullable #1119

nym3r0s opened this issue Nov 8, 2022 · 12 comments · Fixed by #1124
Labels

Comments

@nym3r0s
Copy link

nym3r0s commented Nov 8, 2022

Hello,

This is my first question here, so apologies in advance if I missed any information and pls let me know if more info is needed.

We have a table with a column defined as such

 `my_custom_column`                       SimpleAggregateFunction(anyLast, Nullable(DateTime64(3, 'UTC'))),

However, when we attempt to write a row with the column set to a null value, we are getting errors and on debugging it seems like the ClickhouseColumn is set as nullable=false.

Caused by: java.sql.SQLException: Cannot set null to non-nullable column #1 [my_custom_column SimpleAggregateFunction(anyLast, Nullable(DateTime64(3, 'UTC')))]
	at com.clickhouse.jdbc.SqlExceptionUtils.clientError(SqlExceptionUtils.java:73)
	at com.clickhouse.jdbc.internal.InputBasedPreparedStatement.addBatch(InputBasedPreparedStatement.java:328)
	at com.clickhouse.jdbc.internal.InputBasedPreparedStatement.executeAny(InputBasedPreparedStatement.java:110)
	at com.clickhouse.jdbc.internal.InputBasedPreparedStatement.executeLargeUpdate(InputBasedPreparedStatement.java:185)
	at com.clickhouse.jdbc.internal.AbstractPreparedStatement.executeUpdate(AbstractPreparedStatement.java:135)
	at com.zaxxer.hikari.pool.ProxyPreparedStatement.executeUpdate(ProxyPreparedStatement.java:61)
	at com.zaxxer.hikari.pool.HikariProxyPreparedStatement.executeUpdate(HikariProxyPreparedStatement.java)
	at org.hibernate.engine.jdbc.internal.ResultSetReturnImpl.executeUpdate(ResultSetReturnImpl.java:197)

I am able to insert a NULL value for the column via the clickhouse client CLI - so this seems like an issue with the Java driver.
More specifically - an error parsing the column definition from the Database.

Can you please help? Thanks!

@zhicwu zhicwu added the bug label Nov 8, 2022
@zhicwu
Copy link
Contributor

zhicwu commented Nov 8, 2022

Hi @nym3r0s, thanks for the bug report. For SimpleAggregateFunction, we should use the inner type, so it's actually nullable column in this case.

@zhicwu
Copy link
Contributor

zhicwu commented Nov 9, 2022

I think SimpleAggregateFunction(anyLast, Nullable(DateTime64(3, 'UTC'))) and Nullable(SimpleAggregateFunction(anyLast, DateTime64(3, 'UTC'))) are the same. And it looks like ClickHouse prefers the latter, as you can tell from the result of select toTypeName(anyLastSimpleState(null::Nullable(DateTime64(3, 'UTC')))).

Anyway, both cases work now in my local and I'll push the changes to develop branch soon. You may take Nullable(SimpleAggregateFunction) as a workaround, or try out nightly build a few hours later.

@nym3r0s
Copy link
Author

nym3r0s commented Nov 9, 2022

Thanks! @zhicwu

@nym3r0s
Copy link
Author

nym3r0s commented Nov 9, 2022

@zhicwu - I'm not sure this is true for all cases. For example

This causes a syntax error:

CREATE TABLE IF NOT EXISTS mytable (
    `id` UInt64,
     `my_custom_column`             Nullable(SimpleAggregateFunction(anyLast, LowCardinality(String)))
) ENGINE = MergeTree
primary key `id`
order by `id`;

And this doesn't cause a syntax error.

CREATE TABLE IF NOT EXISTS mytable (
    `id` UInt64,
     `my_custom_column`             SimpleAggregateFunction(anyLast, LowCardinality(Nullable(String)))
) ENGINE = MergeTree
primary key `id`
order by `id`;

@zhicwu
Copy link
Contributor

zhicwu commented Nov 9, 2022

Good point but LowCardinality is simply ignored(because the column only keeps one single value?):

-- [NULL]	LowCardinality(Nullable(String))
select null::LowCardinality(Nullable(String)) v , toTypeName(v) t

-- [NULL]	Nullable(SimpleAggregateFunction(anyLast, String))
select anyLastSimpleState(v) x, toTypeName(x) y
from (select null::LowCardinality(Nullable(String)) v , toTypeName(v) t)

Having said that, I've just verified the local changes and it works well :)

@zhicwu zhicwu linked a pull request Nov 10, 2022 that will close this issue
@nym3r0s
Copy link
Author

nym3r0s commented Nov 14, 2022

@zhicwu - our use-case here is a field that can have a small set of values which includes null - example - NULL / A / B / C - which to me (admittedly new to clickhouse) seems to be a LowCardinality(Nullable(String)).

Am I missing/misunderstanding something, please let me know 🙏 🙂

Also, when the next release to the JDBC driver expected? We're planning to use this to serve customers, so would prefer something more stable than a nightly build. If the nightly build is fairly stable, we can consider using the nightly build 👍

@zhicwu
Copy link
Contributor

zhicwu commented Nov 15, 2022

our use-case here is a field that can have a small set of values which includes null - example - NULL / A / B / C - which to me (admittedly new to clickhouse) seems to be a LowCardinality(Nullable(String)).

If you're talking about fixed values, you may want to try Enum8/Enum16. And if it's always about one single character, you may use FixedString(1). As to null value, it's trival for most people, but to me it introduced unnecessary overhead in serialization and deserialization when data format is Native or RowBinary, so my take was to create a default value to avoid that.

Also, when the next release to the JDBC driver expected? We're planning to use this to serve customers, so would prefer something more stable than a nightly build. If the nightly build is fairly stable, we can consider using the nightly build 👍

I promised v0.3.3 release in August or so, but now it's Nov and we still have 60%+ issues to resolve... I'll squeeze more time for this project and release 0.3.3 before new year.

Understood nightly build is scary for production usage, but since we only have a few unit test and integration test here, I have to say it's same as release. Having said that, it's usually good idea to use latest patch release like v0.3.2-patch11 and only use snapshot when you have to. If you're taking the latter approach, it is strongly suggested to publish the artifact in your own maven repository to protect your application from unexpected breaking changes.

@nym3r0s
Copy link
Author

nym3r0s commented Dec 22, 2022

Hi @zhicwu - just following up on this - any luck with getting a new release out?

@zhicwu
Copy link
Contributor

zhicwu commented Dec 23, 2022

Hi @zhicwu - just following up on this - any luck with getting a new release out?

Apologize for taking so long. Have you tried nightly build to verify your case? I'll update roadmap and wrap up what we have in these days and release v0.3.3.

@nym3r0s
Copy link
Author

nym3r0s commented Dec 23, 2022

We're not allowed to publish nightly builds into our internal maven repo unfortunately, So I can't really test a build in production. 😞

For now, I've worked around this by sending the data via HTTP but I'd like to use JDBC at some point.

@zhicwu
Copy link
Contributor

zhicwu commented Dec 23, 2022

I'm not suggesting to try that on production right away. It's very common to test nightly build in a sandbox(e.g. local/sit/uat environment) first to verify if a feature or bug fix is ready.

Anyway, it's fine to wait for the release.

@kfy971102
Copy link

kfy971102 commented Jan 22, 2024

I'm not suggesting to try that on production right away. It's very common to test nightly build in a sandbox(e.g. local/sit/uat environment) first to verify if a feature or bug fix is ready.

Anyway, it's fine to wait for the release.

The problem that null values cannot be inserted has been solved in v0.3.2-patch11? I tried to introduce this version yes, but the problem that simpleaggregatefunction (anylast, nullable (string)) cannot insert null values already exists. Has it been solved? I look forward to your answe @zhicwu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants