Skip to content

Conversation

@mayur-bhindi
Copy link

@mayur-bhindi mayur-bhindi commented Apr 25, 2022

HHH-15228

"RoundFunction" which extends StandardSQLFunction to return BigDecimal

  • support for round() function in H2 database to return BigDecimal if argument is BigDecimal
  • By default, it would return Double

@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Apr 25, 2022

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@mayur-bhindi mayur-bhindi changed the title HHH-15228 HHH-15228 ("RoundFunction" which extends StandardSQLFunction to return BigDecimal) Apr 25, 2022
mayur added 2 commits April 25, 2022 18:33
"RoundFunction" which extends StandardSQLFunction to return BigDecimal if argument is BigDecimal, otherwise it returns Decimal.
"RoundFunction" which extends StandardSQLFunction to return BigDecimal if argument is BigDecimal, otherwise it returns Double.
@mayur-bhindi mayur-bhindi marked this pull request as draft April 27, 2022 17:56
@mayur-bhindi mayur-bhindi marked this pull request as ready for review April 27, 2022 17:56
return firstArgumentType == StandardBasicTypes.BIG_DECIMAL ? firstArgumentType : StandardBasicTypes.DOUBLE;
}

}
Copy link
Contributor

@NathanQingyangXu NathanQingyangXu May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a general rule, PR without testing case has slim chance to be approved (with the exception of doc PR), let alone merged.
Please consider creating a compelling testing case(s) to prove your code changes. It would help both reviewer and reviewee at the same time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @NathanQingyangXu I have added unit tests for same.

mayur added 3 commits May 4, 2022 15:09
Corrected the code format
Corrected the code format (removed extra spaces)
added unit tests
@gavinking
Copy link
Member

Not sure I think we should be making piecemeal changes like this to Hibernate 5.

This is all already fixed Hibernate 6 where all this is much more well-defined, and if you're stuck on 5 you can pretty do this customization in your own project.

added unit tests
@katzyn
Copy link

katzyn commented May 7, 2022

ROUND() in modern versions of H2 (2.0.202 and newer) actually returns value of exactly the same data type as data type of its first argument, if it is a TINYINT, SMALLINT, INTEGER, BIGINT, REAL, DOUBLE PRECISION or DECFLOAT. If it is a NUMERIC (DECIMAL), this function returns a NUMERIC value with adjusted precision and scale.

In unsupported (on H2 side) H2 1.4.200 it returns value of the same data type for REAL and DOUBLE PRECISION arguments and DECIMAL (NUMERIC) for arguments of other data types.

In unsupported (on H2 side) H2 1.4.199 and older versions it returns a DOUBLE PRECISION value unconditionally.

Old implementation looks like correct for H2 1.4.199 and older versions and incorrect for newer versions. Implementation proposed here looks like incorrect for all versions of H2, but it may work better anyway.

@gavinking
Copy link
Member

But the correct (portable) behavior is now defined by JPA, and is not something that is supposed to vary between databases.

Again, I don't think we need to be messing with the existing behavior in H5. This was all already carefully reviewed for H6.

@mayur-bhindi
Copy link
Author

@gavinking but it behaves differently with postgres. We are using postgres database production environment, it returns the data type same as argument, but H2 doesn't for same version of hibernate.

So currently, there are different behaviours for H2 and postgres (I assume others database as well).

@gavinking
Copy link
Member

Again: if you're talking about Hibernate 5 here, then yes, correct, there are very many things that are inconsistent between dialects on H5. These have been fixed in Hibernate 6, and I don't think it makes much sense to go back and try to fix them all in H5.

People who want better portability between databases should update to H6.

@gavinking
Copy link
Member

#9323 shows that this works (it has worked since at least H6 and maybe earlier).

@gavinking gavinking closed this Nov 24, 2024
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.

4 participants