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

Reject invalid ASIN and ACOS arguments and fix PK index for other numeric data types #3985

Merged
merged 3 commits into from Jan 26, 2024

Conversation

katzyn
Copy link
Contributor

@katzyn katzyn commented Jan 25, 2024

  1. ASIN and ACOS now throw an exception for out of range arguments as required by the SQL Standard.
  2. Index cursors on delegated PK index now handle Nan, Infinity, -Infinity and large numeric values properly.

Closes #3981.

}
default:
result = v.getLong();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What this is all about? MVPrimaryIndex is a data table itself. It's key is always of type long and values are data rows of type SearchRow. How would you make this new code ever execute?

Copy link
Contributor Author

@katzyn katzyn Jan 26, 2024

Choose a reason for hiding this comment

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

There is a mandatory requirement from the SQL Standard: all numeric data types are comparable with algebraic rules.

H2 (and some other database systems, such as PostgreSQL) additionally support non-finite IEEE values: positive and negative infinities and NaN. For infinities comparison rules are obvious, for NaN they aren't that clear, but H2 behaves exactly as PostgreSQL.

If operand of comparison operation is a numeric column with an index, we usually can't just convert value of other operand into data type of this column, because its data type usually can't hold all possible numeric values.

For example, a query with WHERE ID > 1e100 is perfectly valid for BIGINT column, but BIGINT data type isn't large enough for 1e100, that's why all modern versions of H2 don't perform such conversion.

When we use MVSecondardyIndex, numeric values with different data types (BIGINT from this index and DECFLOAT or whatever else from the search row) can be compared directly.

But MVDelegateIndex is a different thing. It is based on MVPrimaryIndex and there is a real problem: it doesn't use Value instances as its keys. It uses Long objects instead. We can't compare Long with Value, so H2 historically has a quirk for NULL (it isn't changed here). But what can we do with infinities or other large values? The simplest solution is to map negative infinities and values smaller than Long.MIN_VALUE to Long.MIN_VALUE and positive infinities, NaNs and values larger than Long.MAX_VALUE to Long.MAX_VALUE. The only drawback of such mapping is a possible selection of rows with these boundary values when these rows aren't required, but it is cheap enough.

These cases can be improved: we can return an empty cursor if there is nothing to select, but I think it will require many additional conditions in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

The simplest solution is to map negative infinities and values smaller than Long.MIN_VALUE to Long.MIN_VALUE and positive infinities, NaNs and values larger than Long.MAX_VALUE to Long.MAX_VALUE. The only drawback of such mapping is a possible selection of rows with these boundary values when these rows aren't required, but it is cheap enough.

As an accountant and engineer I can only ask: Please let's not do that! If I understand this correctly, 'NaN" will suddenly match Long.MAX_VALUE and if my table actually holds a value Long.MAX_VALUE we would return a match?! This was a typical Boing 737 Max condition!

I know, chances this to happen are slim, but impact when it happens may be very big.
So the "Best Estimate" may still be very big despite the small odds.

We should never return wrong result sets, but better throw an exception. Because then I have a chance to know that I am on my own.

(Similar to the WITH ... returning no records when having parameters: an exception about unsupported feature would be much better than wrongly no records.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it will not be returned. Index cursors created with index conditions may return additional rows, each row will be re-tested anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok then, thank you for re-assurance!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote this method, new implementation returns an empty cursor when possible. It also avoids creation of lower and upper bounds for MVStore cursor when these sides aren't limited by index condition or when lower condition is smaller than any BIGINT value or upper condition is larger than any BIGINT value. New implementation has many branches, but they are covered by unit tests.

@katzyn katzyn merged commit 6f727b5 into h2database:master Jan 26, 2024
2 checks passed
@katzyn katzyn deleted the nan branch January 26, 2024 13:48
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.

Unexpected result when using trigonomeric functions
3 participants