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

fix: skip negative scale checks for creating decimals #723

Merged
merged 5 commits into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public class CometDictionary implements AutoCloseable {
private final int numValues;

/** Decoded dictionary values. We only need to copy values for decimal type. */
private ByteArrayWrapper[] binaries;
private volatile ByteArrayWrapper[] binaries;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not directly related but forgot to add based on #705 (comment)


public CometDictionary(CometPlainVector values) {
this.values = values;
Expand Down
51 changes: 34 additions & 17 deletions common/src/main/java/org/apache/comet/vector/CometVector.java
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,19 @@ public abstract class CometVector extends ColumnVector {
private final byte[] DECIMAL_BYTES = new byte[DECIMAL_BYTE_WIDTH];
protected final boolean useDecimal128;

private static final long decimalValOffset;

static {
try {
java.lang.reflect.Field unsafeField = sun.misc.Unsafe.class.getDeclaredField("theUnsafe");
unsafeField.setAccessible(true);
final sun.misc.Unsafe unsafe = (sun.misc.Unsafe) unsafeField.get(null);
decimalValOffset = unsafe.objectFieldOffset(Decimal.class.getDeclaredField("decimalVal"));
} catch (Throwable e) {
throw new RuntimeException(e);
}
}

protected CometVector(DataType type, boolean useDecimal128) {
super(type);
this.useDecimal128 = useDecimal128;
Expand Down Expand Up @@ -73,31 +86,35 @@ public boolean isFixedLength() {
@Override
public Decimal getDecimal(int i, int precision, int scale) {
if (!useDecimal128 && precision <= Decimal.MAX_INT_DIGITS() && type instanceof IntegerType) {
return Decimal.createUnsafe(getInt(i), precision, scale);
return createDecimal(getInt(i), precision, scale);
} else if (!useDecimal128 && precision <= Decimal.MAX_LONG_DIGITS()) {
return Decimal.createUnsafe(getLong(i), precision, scale);
return createDecimal(getLong(i), precision, scale);
} else {
byte[] bytes = getBinaryDecimal(i);
BigInteger bigInteger = new BigInteger(bytes);
BigDecimal javaDecimal = new BigDecimal(bigInteger, scale);
try {
return Decimal.apply(javaDecimal, precision, scale);
} catch (ArithmeticException e) {
throw new ArithmeticException(
"Cannot convert "
+ javaDecimal
+ " (bytes: "
+ bytes
+ ", integer: "
+ bigInteger
+ ") to decimal with precision: "
+ precision
+ " and scale: "
+ scale);
}
return createDecimal(javaDecimal, precision, scale);
}
}

/** This method skips the negative scale check, otherwise the same as Decimal.createUnsafe(). */
private Decimal createDecimal(long unscaled, int precision, int scale) {
Decimal dec = new Decimal();
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems the check for negative scale is a feature for ANSI correctness? https://issues.apache.org/jira/browse/SPARK-30252

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not need to do the check every read, we are running ANSI tests in spark_sql_test_ansi.yml

dec.org$apache$spark$sql$types$Decimal$$longVal_$eq(unscaled);
dec.org$apache$spark$sql$types$Decimal$$_precision_$eq(precision);
dec.org$apache$spark$sql$types$Decimal$$_scale_$eq(scale);
return dec;
}

/** This method skips a few checks, otherwise the same as Decimal.apply(). */
private Decimal createDecimal(BigDecimal value, int precision, int scale) {
Decimal dec = new Decimal();
Platform.putObjectVolatile(dec, decimalValOffset, new scala.math.BigDecimal(value));
Copy link
Contributor

Choose a reason for hiding this comment

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

The spark version has some additional checks for rounding and precision. Do we not need those?

    this.decimalVal = decimal.setScale(scale, ROUND_HALF_UP)
    if (decimalVal.precision > precision) {
      throw QueryExecutionErrors.decimalPrecisionExceedsMaxPrecisionError(
        decimalVal.precision, precision)
    }

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 we do not need the precision check because it is done in advance

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my knowledge, where?

dec.org$apache$spark$sql$types$Decimal$$_precision_$eq(precision);
dec.org$apache$spark$sql$types$Decimal$$_scale_$eq(scale);
return dec;
}

/**
* Reads a 16-byte byte array which are encoded big-endian for decimal128 into internal byte
* array.
Expand Down
1 change: 0 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,6 @@ under the License.
</execution>
</executions>
<configuration>
-->
<scalaVersion>${scala.version}</scalaVersion>
<checkMultipleScalaVersions>true</checkMultipleScalaVersions>
<failOnMultipleScalaVersions>true</failOnMultipleScalaVersions>
Expand Down
Loading