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.

Sign up for GitHub

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

Jump to bottom

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
186647d
Throw exception on invalid ASIN or ACOS argument as requried by the S…
katzyn Jan 25, 2024
e1cabe3
Fix MVPrimaryIndex.extractPKFromRow() for other numeric data types
katzyn Jan 25, 2024
52d0501
Remove quirks from MVPrimaryIndex.find() and add more tests
katzyn Jan 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions h2/src/docsrc/html/changelog.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ <h1>Change Log</h1>

<h2>Next Version (unreleased)</h2>
<ul>
<li>Issue #3981: Unexpected result when using trigonometric functions
</li>
<li>Issue #3983: INVISIBLE columns should be ignored in INSERT statement without explicit column list
</li>
<li>Issue #3960: NullPointerException when executing batch insert
Expand Down
6 changes: 6 additions & 0 deletions h2/src/main/org/h2/expression/function/MathFunction1.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,15 @@ public Value getValue(SessionLocal session) {
d = Math.tanh(d);
break;
case ASIN:
if (d < -1d || d > 1d) {
throw DbException.getInvalidValueException("ASIN() argument", d);
}
d = Math.asin(d);
break;
case ACOS:
if (d < -1d || d > 1d) {
throw DbException.getInvalidValueException("ACOS() argument", d);
}
d = Math.acos(d);
break;
case ATAN:
Expand Down
5 changes: 3 additions & 2 deletions h2/src/main/org/h2/index/RangeIndex.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,9 @@ public Cursor findFirstOrLast(SessionLocal session, boolean first) {
if (step == 0L) {
throw DbException.get(ErrorCode.STEP_SIZE_MUST_NOT_BE_ZERO);
}
return new SingleRowCursor((step > 0 ? min <= max : min >= max)
? Row.get(new Value[]{ ValueBigint.get(first ^ min >= max ? min : max) }, 1) : null);
return (step > 0 ? min <= max : min >= max)
? new SingleRowCursor(Row.get(new Value[] { ValueBigint.get(first ^ min >= max ? min : max) }, 1))
: SingleRowCursor.EMPTY;
}

@Override
Expand Down
6 changes: 6 additions & 0 deletions h2/src/main/org/h2/index/SingleRowCursor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@
* A cursor with at most one row.
*/
public class SingleRowCursor implements Cursor {

/**
* An empty cursor.
*/
public static final SingleRowCursor EMPTY = new SingleRowCursor(null);

private Row row;
private boolean end;

Expand Down
124 changes: 94 additions & 30 deletions h2/src/main/org/h2/mvstore/db/MVPrimaryIndex.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.h2.mvstore.db;

import java.math.BigDecimal;
import java.util.List;
import java.util.Map.Entry;
import java.util.concurrent.atomic.AtomicLong;
Expand All @@ -29,8 +30,8 @@
import org.h2.table.IndexColumn;
import org.h2.table.TableFilter;
import org.h2.value.Value;
import org.h2.value.ValueDecfloat;
import org.h2.value.ValueLob;
import org.h2.value.ValueNull;
import org.h2.value.VersionedValue;

/**
Expand Down Expand Up @@ -230,31 +231,101 @@ private Row lockRow(TransactionMap<Long,SearchRow> map, long key, int timeoutMil

@Override
public Cursor find(SessionLocal session, SearchRow first, SearchRow last) {
long min = extractPKFromRow(first, Long.MIN_VALUE);
long max = extractPKFromRow(last, Long.MAX_VALUE);
return find(session, min, max);
}

private long extractPKFromRow(SearchRow row, long defaultValue) {
long result;
if (row == null) {
result = defaultValue;
} else if (mainIndexColumn == SearchRow.ROWID_INDEX) {
result = row.getKey();
Long min, max;
Value v;
if (first == null) {
min = null;
} else if (mainIndexColumn == SearchRow.ROWID_INDEX || (v = first.getValue(mainIndexColumn)) == null) {
min = first.getKey();
} else {
Value v = row.getValue(mainIndexColumn);
if (v == null) {
result = row.getKey();
} else if (v == ValueNull.INSTANCE) {
result = 0L;
} else {
result = v.getLong();
switch (v.getValueType()) {
case Value.NULL:
return SingleRowCursor.EMPTY;
case Value.REAL:
case Value.DOUBLE: {
double d = v.getDouble();
if (Double.isNaN(d)) {
return SingleRowCursor.EMPTY;
} else {
min = (long) d;
}
break;
}
case Value.DECFLOAT:
if (!((ValueDecfloat) v).isFinite()) {
if (v == ValueDecfloat.NEGATIVE_INFINITY) {
min = null;
} else {
return SingleRowCursor.EMPTY;
}
break;
}
//$FALL-THROUGH$
case Value.NUMERIC: {
BigDecimal bd = v.getBigDecimal();
if (bd.compareTo(Value.MAX_LONG_DECIMAL) > 0) {
return SingleRowCursor.EMPTY;
} else if (bd.compareTo(Value.MIN_LONG_DECIMAL) < 0) {
min = null;
} else {
min = bd.longValue();
}
break;
}
default:
min = v.getLong();
}
}
return result;
if (last == null) {
max = null;
} else if (mainIndexColumn == SearchRow.ROWID_INDEX || (v = last.getValue(mainIndexColumn)) == null) {
max = last.getKey();
} else {
switch (v.getValueType()) {
case Value.NULL:
return SingleRowCursor.EMPTY;
case Value.REAL:
case Value.DOUBLE: {
double d = v.getDouble();
if (Double.isNaN(d)) {
max = null;
} else {
max = (long) d;
}
break;
}
case Value.DECFLOAT:
if (!((ValueDecfloat) v).isFinite()) {
if (v == ValueDecfloat.NEGATIVE_INFINITY) {
return SingleRowCursor.EMPTY;
} else {
max = null;
}
break;
}
//$FALL-THROUGH$
case Value.NUMERIC: {
BigDecimal bd = v.getBigDecimal();
if (bd.compareTo(Value.MAX_LONG_DECIMAL) > 0) {
max = null;
} else if (bd.compareTo(Value.MIN_LONG_DECIMAL) < 0) {
return SingleRowCursor.EMPTY;
} else {
max = bd.longValue();
}
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 •
edited

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.

break;
}
default:
max = v.getLong();
}
}
TransactionMap<Long,SearchRow> map = getMap(session);
if (min != null && max != null && min.longValue() == max.longValue()) {
return new SingleRowCursor(setRowKey((Row) map.getFromSnapshot(min), min));
}
return new MVStoreCursor(map.entryIterator(min, max));
}


@Override
public MVTable getTable() {
return mvTable;
Expand Down Expand Up @@ -319,7 +390,8 @@ public boolean canGetFirstOrLast() {
public Cursor findFirstOrLast(SessionLocal session, boolean first) {
TransactionMap<Long, SearchRow> map = getMap(session);
Entry<Long, SearchRow> entry = first ? map.firstEntry() : map.lastEntry();
return new SingleRowCursor(entry != null ? setRowKey((Row) entry.getValue(), entry.getKey()) : null);
return entry != null ? new SingleRowCursor(setRowKey((Row) entry.getValue(), entry.getKey()))
: SingleRowCursor.EMPTY;
}

@Override
Expand Down Expand Up @@ -360,14 +432,6 @@ public void addBufferedRows(List<String> bufferNames) {
throw new UnsupportedOperationException();
}

private Cursor find(SessionLocal session, Long first, Long last) {
TransactionMap<Long,SearchRow> map = getMap(session);
if (first != null && last != null && first.longValue() == last.longValue()) {
return new SingleRowCursor(setRowKey((Row) map.getFromSnapshot(first), first));
}
return new MVStoreCursor(map.entryIterator(first, last));
}

@Override
public boolean isRowIdIndex() {
return true;
Expand Down
2 changes: 1 addition & 1 deletion h2/src/main/org/h2/mvstore/db/MVSecondaryIndex.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ public Cursor findFirstOrLast(SessionLocal session, boolean first) {
return new SingleRowCursor(mvTable.getRow(session, key.getKey()));
}
}
return new SingleRowCursor(null);
return SingleRowCursor.EMPTY;
}

@Override
Expand Down
48 changes: 26 additions & 22 deletions h2/src/main/org/h2/res/help.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4845,8 +4845,10 @@ ABS(CAST(I AS BIGINT))
ACOS(numeric)
","
Calculate the arc cosine.
See also Java ""Math.acos"".
This method returns a double.

Argument must be between -1 and 1 inclusive.

This function returns a double precision value.
","
ACOS(D)
"
Expand All @@ -4855,8 +4857,10 @@ ACOS(D)
ASIN(numeric)
","
Calculate the arc sine.
See also Java ""Math.asin"".
This method returns a double.

Argument must be between -1 and 1 inclusive.

This function returns a double precision value.
","
ASIN(D)
"
Expand All @@ -4865,8 +4869,8 @@ ASIN(D)
ATAN(numeric)
","
Calculate the arc tangent.
See also Java ""Math.atan"".
This method returns a double.

This function returns a double precision value.
","
ATAN(D)
"
Expand All @@ -4875,8 +4879,8 @@ ATAN(D)
COS(numeric)
","
Calculate the trigonometric cosine.
See also Java ""Math.cos"".
This method returns a double.

This function returns a double precision value.
","
COS(ANGLE)
"
Expand All @@ -4885,8 +4889,8 @@ COS(ANGLE)
COSH(numeric)
","
Calculate the hyperbolic cosine.
See also Java ""Math.cosh"".
This method returns a double.

This function returns a double precision value.
","
COSH(X)
"
Expand All @@ -4895,8 +4899,8 @@ COSH(X)
@h2@ COT(numeric)
","
Calculate the trigonometric cotangent (""1/TAN(ANGLE)"").
See also Java ""Math.*"" functions.
This method returns a double.

This function returns a double precision value.
","
COT(ANGLE)
"
Expand All @@ -4905,8 +4909,8 @@ COT(ANGLE)
SIN(numeric)
","
Calculate the trigonometric sine.
See also Java ""Math.sin"".
This method returns a double.

This function returns a double precision value.
","
SIN(ANGLE)
"
Expand All @@ -4915,8 +4919,8 @@ SIN(ANGLE)
SINH(numeric)
","
Calculate the hyperbolic sine.
See also Java ""Math.sinh"".
This method returns a double.

This function returns a double precision value.
","
SINH(ANGLE)
"
Expand All @@ -4925,8 +4929,8 @@ SINH(ANGLE)
TAN(numeric)
","
Calculate the trigonometric tangent.
See also Java ""Math.tan"".
This method returns a double.

This function returns a double precision value.
","
TAN(ANGLE)
"
Expand All @@ -4935,8 +4939,8 @@ TAN(ANGLE)
TANH(numeric)
","
Calculate the hyperbolic tangent.
See also Java ""Math.tanh"".
This method returns a double.

This function returns a double precision value.
","
TANH(X)
"
Expand All @@ -4945,8 +4949,8 @@ TANH(X)
@h2@ ATAN2(numeric, numeric)
","
Calculate the angle when converting the rectangular coordinates to polar coordinates.
See also Java ""Math.atan2"".
This method returns a double.

This function returns a double precision value.
","
ATAN2(X, Y)
"
Expand Down
7 changes: 5 additions & 2 deletions h2/src/main/org/h2/value/Value.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,10 +378,13 @@ public abstract class Value extends VersionedValue<Value> implements HasSQL, Typ

private static SoftReference<Value[]> softCache;

static final BigDecimal MAX_LONG_DECIMAL = BigDecimal.valueOf(Long.MAX_VALUE);
/**
* The largest BIGINT value, as a BigDecimal.
*/
public static final BigDecimal MAX_LONG_DECIMAL = BigDecimal.valueOf(Long.MAX_VALUE);

/**
* The smallest Long value, as a BigDecimal.
* The smallest BIGINT value, as a BigDecimal.
*/
public static final BigDecimal MIN_LONG_DECIMAL = BigDecimal.valueOf(Long.MIN_VALUE);

Expand Down
4 changes: 2 additions & 2 deletions h2/src/test/org/h2/test/db/TestTableEngines.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,8 +1007,8 @@ public boolean canGetFirstOrLast() {

@Override
public Cursor findFirstOrLast(SessionLocal session, boolean first) {
return new SingleRowCursor((Row)
(set.isEmpty() ? null : first ? set.first() : set.last()));
return set.isEmpty() ? SingleRowCursor.EMPTY
: new SingleRowCursor((Row) (first ? set.first() : set.last()));
}

@Override
Expand Down
9 changes: 9 additions & 0 deletions h2/src/test/org/h2/test/scripts/functions/numeric/acos.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,12 @@ select acos(null) vn, acos(-1) r1;
> ---- -----------------
> null 3.141592653589793
> rows: 1

SELECT ACOS(-1.1);
> exception INVALID_VALUE_2

SELECT ACOS(1.1);
> exception INVALID_VALUE_2

SELECT ACOS(CAST('Infinity' AS DOUBLE PRECISION));
> exception INVALID_VALUE_2