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

Rounding #864

Merged
merged 4 commits into from Mar 13, 2022
Merged

Rounding #864

merged 4 commits into from Mar 13, 2022

Conversation

volodya-lombrozo
Copy link

I have faced with a problem of getting BigDecimal values from ResultSet:

resultSet.getBigDecimal("avg")

For example you can set the avg value = 1.333334 to a cloumn

`abg` Float64

And when you will try to get it you will have the next exception:

java.lang.ArithmeticException: Rounding necessary

	at java.base/java.math.BigDecimal.commonNeedIncrement(BigDecimal.java:4621)
	at java.base/java.math.BigDecimal.needIncrement(BigDecimal.java:4677)
	at java.base/java.math.BigDecimal.divideAndRound(BigDecimal.java:4585)
	at java.base/java.math.BigDecimal.setScale(BigDecimal.java:2891)
	at java.base/java.math.BigDecimal.setScale(BigDecimal.java:2951)
	at com.clickhouse.client.data.ClickHouseDoubleValue.asBigDecimal(ClickHouseDoubleValue.java:140)
	at com.clickhouse.client.ClickHouseValue.asBigDecimal(ClickHouseValue.java:234)
	at com.clickhouse.jdbc.ClickHouseResultSet.getBigDecimal(ClickHouseResultSet.java:237)

I added RoundingMode in order to handle decimal values

@CLAassistant
Copy link

CLAassistant commented Mar 11, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link

Benchmark                                 (client)  (connection)  (statement)  (type)   Mode  Cnt     Score     Error  Units
Basic.insertOneRandomNumber  clickhouse-http-jdbc1         reuse       normal  object  thrpt   20   219.917 ±  26.999  ops/s
Basic.insertOneRandomNumber  clickhouse-http-jdbc1         reuse     prepared  object  thrpt   20   227.671 ±  23.889  ops/s
Basic.insertOneRandomNumber  clickhouse-http-jdbc1           new       normal  object  thrpt   20   216.784 ±  26.931  ops/s
Basic.insertOneRandomNumber  clickhouse-http-jdbc1           new     prepared  object  thrpt   20   220.458 ±  25.412  ops/s
Basic.insertOneRandomNumber   clickhouse-grpc-jdbc         reuse       normal  object  thrpt   20   232.792 ±  25.948  ops/s
Basic.insertOneRandomNumber   clickhouse-grpc-jdbc           new       normal  object  thrpt   20   228.667 ±  28.628  ops/s
Basic.selectOneRandomNumber  clickhouse-http-jdbc1         reuse       normal  object  thrpt   20  1212.455 ± 138.125  ops/s
Basic.selectOneRandomNumber  clickhouse-http-jdbc1         reuse     prepared  object  thrpt   20  1262.955 ±  99.241  ops/s
Basic.selectOneRandomNumber  clickhouse-http-jdbc1           new       normal  object  thrpt   20  1209.858 ±  91.764  ops/s
Basic.selectOneRandomNumber  clickhouse-http-jdbc1           new     prepared  object  thrpt   20  1280.747 ±  70.558  ops/s
Basic.selectOneRandomNumber   clickhouse-grpc-jdbc         reuse       normal  object  thrpt   20   785.802 ±  87.088  ops/s
Basic.selectOneRandomNumber   clickhouse-grpc-jdbc         reuse     prepared  object  thrpt   20   848.482 ±  80.445  ops/s
Basic.selectOneRandomNumber   clickhouse-grpc-jdbc           new       normal  object  thrpt   20   834.440 ±  85.877  ops/s
Basic.selectOneRandomNumber   clickhouse-grpc-jdbc           new     prepared  object  thrpt   20   841.690 ±  75.414  ops/s

@zhicwu zhicwu changed the base branch from master to develop March 12, 2022 06:00
@zhicwu
Copy link
Contributor

zhicwu commented Mar 13, 2022

Thank you @volodya-lombrozo for the pull request. I changed the merge target from master to develop, and I'll merge it after double checking the rounding mode.

@zhicwu
Copy link
Contributor

zhicwu commented Mar 13, 2022

@volodya-lombrozo, I hope you don't mind that I changed RoundingMode to DOWN for consistency with server. Also I removed the check for NaN and Infinite because BigDecimal does not support them.

I'm going to release patch7 24 hours later - let me know if you have any concern for the changes.

@zhicwu zhicwu merged commit 159e02d into ClickHouse:develop Mar 13, 2022
@volodya-lombrozo
Copy link
Author

Ok, thank you, @zhicwu. Everything fine, but can I ask you, why you decided to use exactly RoundingMode.DOWN?
For example in that line you use RoundingMode.HALF_UP.
Maybe it makes sense to create a global setting like DEFAULT_ROUNDING_MODE and use it everywhere?

@zhicwu
Copy link
Contributor

zhicwu commented Mar 14, 2022

why you decided to use exactly RoundingMode.DOWN?

Just trying to make it consistency with server side implementation - see here.

For example in that line you use RoundingMode.HALF_UP.

It's legacy driver and it will be removed in 0.4.0 so I didn't change the logic this time.

Maybe it makes sense to create a global setting like DEFAULT_ROUNDING_MODE and use it everywhere?

Yes. I thought about adding new default into ClickHouseDefaults but maybe better do that in a feature release like v0.3.3? Will create a separate PR for this.

@volodya-lombrozo
Copy link
Author

Thank you for your answer. Now it's clear.
Yes, I think it's much better to add the new default into ClickHouseDefaults in a separate PR.

rernas35 pushed a commit to rernas35/clickhouse-jdbc that referenced this pull request May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants