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

Potential issue when using ROUND #3982

Open
suyZhong opened this issue Jan 25, 2024 · 11 comments
Open

Potential issue when using ROUND #3982

suyZhong opened this issue Jan 25, 2024 · 11 comments
Assignees

Comments

@suyZhong
Copy link

Consider the following test cases, the queries execute slowly.

sql> SELECT ROUND(1, -9999999);
0
0
(1 row, 2717 ms)
sql> SELECT ROUND(1, -99999999);
0
0
(1 row, 74565 ms)

I understand that the negative value for the second argument of ROUND is somewhat illegal. However, in SQLite, MySQL and PostgreSQL, the queries take less than 1 second.

I originally find this by building the latest source version cd09d93. I could also reproduce this in 2.2.224.

Here's how I start H2 Shell:

java -cp target/h2-2.2.229-SNAPSHOT.jar org.h2.tools.Shell -url jdbc:h2:./test -user sa -password 1
@manticore-projects
Copy link
Contributor

I can confirm this problem and have tried to trace it, unfortunately inconclusive:

image

@manticore-projects
Copy link
Contributor

This is the culprit and it is plain Java code:

long scaled = BigDecimal.valueOf(original).setScale(scale, roundingMode).longValue();

@manticore-projects
Copy link
Contributor

manticore-projects commented Jan 25, 2024

Since you want to create a BigDecimal with 10,000 digits I guess you have to deal with the cost. Everything seems to be correct from the code's point of view:

public static void main(String[] args) {
        long t1 = System.nanoTime();
        Long result = BigDecimal.valueOf(1L).setScale(-999999, RoundingMode.HALF_EVEN).longValue();
        System.out.println((System.nanoTime() - t1) * 1E-6);

        t1 = System.nanoTime();
        result = BigDecimal.valueOf(1L).setScale(999999, RoundingMode.HALF_EVEN).longValue();
        System.out.println((System.nanoTime() - t1) * 1E-6);
    }
235.464928
215.282848

I think we can close this issue as "Explained, nothing to do"?

@katzyn
Copy link
Contributor

katzyn commented Jan 25, 2024

I think these functions should reject out of range arguments.

@katzyn
Copy link
Contributor

katzyn commented Jan 25, 2024

Or maybe use optimized code paths for them if we really have a reason to support them.

@manticore-projects
Copy link
Contributor

manticore-projects commented Jan 25, 2024

I think these functions should reject out of range arguments.

I can craft a PR if you like (I though about this at first), but why exactly?
Round(x, 99999) is valid and takes as long as Round(x, -99999) and both work correctly (because the challenge is creating the BigDecimal only, not about rounding).

Your call please.

@katzyn
Copy link
Contributor

katzyn commented Jan 25, 2024

Values larger than ValueNumeric.MAXIMUM_SCALE or smaller than - ValueNumeric.MAXIMUM_SCALE don't look right.

They can throw something like this (for simplicity):

throw DbException.get(ErrorCode.INVALID_VALUE_SCALE, Integer.toString(scale), "-" + MAXIMUM_SCALE, "" + MAXIMUM_SCALE);

The TRUNC() function is also affected.

@katzyn
Copy link
Contributor

katzyn commented Jan 25, 2024

Actually everything depends on data type of the first argument, so this range also isn't that good.

@suyZhong
Copy link
Author

suyZhong commented Feb 2, 2024

Hi! Thanks for all the detailed explanations. I am curious to know if there has been any update regarding this issue. Is it considered as an expected behavior or so?

I'm currently developing an automated testing tool and if such cases would be regarded as expected, I'll try to block these stuffs.

@manticore-projects
Copy link
Contributor

Greetings.

@katzyn: In my opinion, everything is correct here since the time is only consumed by creating the BigDecimal with many digits (positive and negative).

Can we close this?

@katzyn
Copy link
Contributor

katzyn commented Feb 13, 2024

No, there is a bigger problem with these functions, both implementations and documentation need to be improved.

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

No branches or pull requests

3 participants