andygrove commented on code in PR #6730:
URL: https://github.com/apache/datafusion-comet/pull/6730#discussion_r4210961605
##########
spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/ArrowWriters.scala:
##########
@@ -1031,31 +1190,57 @@ private[arrow] class DoubleWriter(val valueVector:
Float8Vector)
* that `getDecimal` would build is only built for a value that does not fit,
to fail as before.
*/
private[arrow] class DecimalWriter(val valueVector: DecimalVector, precision:
Int, scale: Int)
- extends FixedWidthArrowFieldWriter {
+ extends FixedWidthArrowFieldWriter(valueVector) {
import ArrowFieldWriter.LittleEndian
override def setValue(input: SpecializedGetters, ordinal: Int): Unit = {
ensureCapacity(count + 1)
setValueUnsafe(input, ordinal)
}
+ // Spark's unsafe rows and arrays hold up to 18 digits as the unscaled long,
and more as the
+ // unscaled bytes, which are read in place. An unsafe row's getDecimal does
not check the long
+ // against the precision, but an unsafe array's does, so a long that does
not fit is left to it.
override protected def setValueUnsafe(input: SpecializedGetters, ordinal:
Int): Unit = {
- if (precision > Decimal.MAX_LONG_DIGITS && LittleEndian &&
- (input.isInstanceOf[UnsafeRow] || input.isInstanceOf[UnsafeArrayData])) {
- val bytes = input.getBinary(ordinal)
- if (putBigEndianIfFits(count, bytes, Platform.BYTE_ARRAY_OFFSET.toLong,
bytes.length)) {
- BitVectorHelper.setBit(valueVector.getValidityBuffer, count.toLong)
- return
- }
- }
- val decimal = input.getDecimal(ordinal, precision, scale)
- if (decimal.changePrecision(precision, scale)) {
- setUnscaled(count, decimal)
+ val written = LittleEndian && (input match {
+ case row: UnsafeRow if precision <= Decimal.MAX_LONG_DIGITS =>
+ putLong(valueAddress, row.getLong(ordinal))
+ true
+ case array: UnsafeArrayData if precision <= Decimal.MAX_LONG_DIGITS =>
+ val unscaled = array.getLong(ordinal)
+ val fits = DecimalWriter.fitsPrecision(unscaled >> 63, unscaled,
precision)
+ if (fits) {
+ putLong(valueAddress, unscaled)
+ }
+ fits
Review Comment:
Thanks, added in b441fe5148 along the lines of yours. It uses 10^p and
-10^p, the smallest values past the precision, rather than 1234567.89 and
123456789012345678901. With the check here replaced by `true`, or with the wide
branch ignoring what `putUnsafeBytesIfFits` returns, only the new test fails.
The same goes for an off-by-one at 10^p or a negation without the `+ 1` in
`fitsPrecision`, which none of the three suites caught before.
I also took the #6729 paragraph out of the description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
