dongjoon-hyun commented on code in PR #58842:
URL: https://github.com/apache/spark/pull/58842#discussion_r4496421579
##########
common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java:
##########
@@ -194,6 +205,74 @@ public static UTF8String blankString(int length) {
return fromBytes(spaces);
}
+ /**
+ * Creates an UTF8String holding the base-10 representation of a long,
equivalent to
+ * `fromString(Long.toString(value))` but without allocating an intermediate
`java.lang.String`.
+ * Callers with narrower integral types (byte, short, int) can widen to long
without changing the
+ * result.
+ */
+ public static UTF8String fromLong(long value) {
+ if (value == 0) {
+ return ZERO_UTF8;
Review Comment:
`fromLong(0)` (and `fromBoolean` below) now return JVM-wide shared
instances, and `getBytes()` hands out their backing `byte[]` without copying.
Previously, each row got a fresh buffer. For example, in `cast(cast(b AS
STRING) AS BINARY)` the resulting `byte[]` would be the internal array of
`TRUE_UTF8`, so any downstream code that mutates the binary in place (e.g., a
UDF on `Array[Byte]`) would corrupt every later `cast(true AS STRING)` in the
same JVM.
There is precedent (`hex(0)` and `toBinaryString(0)` already return
`ZERO_UTF8`), so this may be acceptable. Could you confirm that this is
intended?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToStringBase.scala:
##########
@@ -353,6 +360,12 @@ trait ToStringBase { self: UnaryExpression with
TimeZoneAwareExpression =>
(c, evPrim) => code"$evPrim =
UTF8String.fromString($c.toPlainString());"
case _: StringType =>
(c, evPrim) => code"$evPrim = $c;"
+ // Format integral and boolean values directly into a UTF8String,
avoiding an intermediate
+ // java.lang.String. byte/short/int widen to long in the generated code.
+ case ByteType | ShortType | IntegerType | LongType =>
Review Comment:
Could you add expression-level test coverage in `CastSuiteBase`? For
integral-to-string, it only has `checkCast(123, "123")` and `checkCast(123L,
"123")` today (boolean is covered by `cast(true, StringType)`). Since
`checkEvaluation` runs both the interpreted and codegen paths, a few cases like
`TINYINT`/`SMALLINT`, `0` (shared instance), negative values, `Long.MinValue`,
and nested `array<tinyint>` / `map<boolean, int>` would pin down both new
branches, including the byte/short -> long widening in the generated code,
beyond `UTF8StringSuite`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToStringBase.scala:
##########
@@ -199,6 +199,13 @@ trait ToStringBase { self: UnaryExpression with
TimeZoneAwareExpression =>
case _: DecimalType if useDecimalPlainString =>
acceptAny[Decimal](d => UTF8String.fromString(d.toPlainString))
case _: StringType => acceptAny[UTF8String](identity[UTF8String])
+ // Format integral and boolean values directly into a UTF8String, avoiding
an intermediate
Review Comment:
Follow-up idea (not necessarily for this PR): for nested types, each element
still materializes a temporary `byte[]` + `UTF8String` via `fromLong` that is
immediately copied into the `UTF8StringBuilder` and dropped (e.g., the
Array/Map/Struct branches above such as L121, and `writeArrayToStringBuilder` /
`writeMapToStringBuilder` / `writeStructToStringBuilder` in codegen). A
`UTF8StringBuilder.appendLong(long)` that writes the digits directly into the
builder buffer would remove those allocations too.
##########
common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java:
##########
@@ -194,6 +205,74 @@ public static UTF8String blankString(int length) {
return fromBytes(spaces);
}
+ /**
+ * Creates an UTF8String holding the base-10 representation of a long,
equivalent to
+ * `fromString(Long.toString(value))` but without allocating an intermediate
`java.lang.String`.
+ * Callers with narrower integral types (byte, short, int) can widen to long
without changing the
+ * result.
+ */
+ public static UTF8String fromLong(long value) {
Review Comment:
Regarding the benchmark in the PR description: with whole-stage codegen off,
`cast(cast(id + 10000000 as int) as string)` goes from 31.4 to 33.1-33.4 ns/row
(~5-6% slower), while the description says the 8-digit case is within noise.
Since there is a single "before" sample, could you re-measure it? If the
regression is real, an int-specialized path (like `Integer.toString` vs
`Long.toString` in the JDK) might recover it, because byte/short/int values
currently go through the long-based `digitCount` and the long loop check.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToStringBase.scala:
##########
@@ -199,6 +199,13 @@ trait ToStringBase { self: UnaryExpression with
TimeZoneAwareExpression =>
case _: DecimalType if useDecimalPlainString =>
acceptAny[Decimal](d => UTF8String.fromString(d.toPlainString))
case _: StringType => acceptAny[UTF8String](identity[UTF8String])
+ // Format integral and boolean values directly into a UTF8String, avoiding
an intermediate
+ // java.lang.String. byte/short/int widen to long without changing the
decimal representation.
+ case ByteType => acceptAny[Byte](b => UTF8String.fromLong(b.toLong))
+ case ShortType => acceptAny[Short](s => UTF8String.fromLong(s.toLong))
+ case IntegerType => acceptAny[Int](i => UTF8String.fromLong(i.toLong))
+ case LongType => acceptAny[Long](l => UTF8String.fromLong(l))
Review Comment:
This changes the interpreted path from the type-agnostic `o.toString` to
strict unboxing (`BoxesRunTime.unboxToShort`, etc.). If a value's boxed type
does not match the declared type (e.g., a `java.lang.Integer` in a `ShortType`
slot of a hand-built `GenericInternalRow`), this now throws
`ClassCastException`, while it printed `1` before. Arguably that is a contract
violation, and `DateType`/`TimestampType` already unbox the same way, but the
four integral cases can be collapsed into one (matching the codegen side at
L365) while keeping the old tolerance:
```suggestion
case ByteType | ShortType | IntegerType | LongType =>
acceptAny[java.lang.Number](n => UTF8String.fromLong(n.longValue))
```
##########
common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java:
##########
@@ -194,6 +205,74 @@ public static UTF8String blankString(int length) {
return fromBytes(spaces);
}
+ /**
+ * Creates an UTF8String holding the base-10 representation of a long,
equivalent to
+ * `fromString(Long.toString(value))` but without allocating an intermediate
`java.lang.String`.
+ * Callers with narrower integral types (byte, short, int) can widen to long
without changing the
+ * result.
+ */
+ public static UTF8String fromLong(long value) {
+ if (value == 0) {
+ return ZERO_UTF8;
+ }
+ boolean negative = value < 0;
+ // Extract digits in negative space so that Long.MIN_VALUE is
representable (it cannot be
+ // negated). For a negative v and q = v / 100, `q * 100 - v` is the
remainder in [0, 99].
+ long v = negative ? value : -value;
+ byte[] bytes = new byte[digitCount(v) + (negative ? 1 : 0)];
+ int pos = bytes.length;
+ // Write two digits per division: in long arithmetic until the value fits
in an int, then in
+ // cheaper int arithmetic.
+ while (v <= Integer.MIN_VALUE) {
+ long q = v / 100;
+ int r = (int) (q * 100 - v);
+ v = q;
+ bytes[--pos] = DIGIT_PAIRS[2 * r + 1];
+ bytes[--pos] = DIGIT_PAIRS[2 * r];
+ }
+ int iv = (int) v;
+ while (iv <= -100) {
+ int q = iv / 100;
+ int r = q * 100 - iv;
+ iv = q;
+ bytes[--pos] = DIGIT_PAIRS[2 * r + 1];
+ bytes[--pos] = DIGIT_PAIRS[2 * r];
+ }
+ // One or two digits are left, iv is in [-99, -1].
+ if (iv <= -10) {
+ bytes[--pos] = DIGIT_PAIRS[-2 * iv + 1];
+ bytes[--pos] = DIGIT_PAIRS[-2 * iv];
+ } else {
+ bytes[--pos] = (byte) ('0' - iv);
+ }
+ if (negative) {
+ bytes[0] = (byte) '-';
+ }
+ return fromBytes(bytes);
Review Comment:
nit: the result is always ASCII (and hence valid UTF-8), but `fromBytes`
leaves `isFullAscii` / `isValid` as `UNKNOWN`, so later calls such as
`substring`, `getChar`, `toUpperCase`, or `isValid()` rescan the bytes.
Pre-setting `FULL_ASCII` / `IS_VALID` here would save that work.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java:
##########
@@ -1610,6 +1610,53 @@ public void toBinaryString() {
UTF8String.toBinaryString(Long.MAX_VALUE));
}
+ @Test
+ public void fromLong() {
+ // Integer.MIN_VALUE and Long.MIN_VALUE are the critical cases: they
cannot be negated, which
+ // is why fromLong extracts digits in negative space. Check their exact
expected text.
+ assertEquals(fromString("-2147483648"),
UTF8String.fromLong(Integer.MIN_VALUE));
+ assertEquals(fromString("-9223372036854775808"),
UTF8String.fromLong(Long.MIN_VALUE));
+ assertEquals(fromString("2147483647"),
UTF8String.fromLong(Integer.MAX_VALUE));
+ assertEquals(fromString("9223372036854775807"),
UTF8String.fromLong(Long.MAX_VALUE));
+
+ // Spot-check the boundaries the digit loops care about: zero, sign, the
switch from long to
+ // int arithmetic around Integer.MIN_VALUE, and the values that cannot be
negated.
+ long[] boundaries = {
+ 0L, 1L, -1L, 9L, -9L, 10L, -10L, 99L, -99L, 100L, -100L, 999L, -999L,
+ Integer.MAX_VALUE, Integer.MIN_VALUE, Integer.MAX_VALUE + 1L,
Integer.MIN_VALUE - 1L,
+ 10000000000L, -10000000000L, Long.MAX_VALUE, Long.MIN_VALUE,
Long.MIN_VALUE + 1
+ };
+ for (long value : boundaries) {
+ assertEquals(fromString(String.valueOf(value)),
UTF8String.fromLong(value));
+ }
+ // Every digit-count change: 10^k - 1 and 10^k, with both signs.
+ long p = 1;
+ for (int k = 1; k <= 18; k++) {
+ p *= 10;
+ for (long value : new long[] {p - 1, p, -(p - 1), -p}) {
+ assertEquals(fromString(String.valueOf(value)),
UTF8String.fromLong(value));
+ }
+ }
+ // Sweep a contiguous range around zero to exercise every small magnitude
and both signs.
+ for (long value = -100000L; value <= 100000L; value++) {
+ assertEquals(fromString(String.valueOf(value)),
UTF8String.fromLong(value));
+ }
+ // Random values across the whole long range.
+ Random random = new Random(42);
+ for (int i = 0; i < 100000; i++) {
+ long value = random.nextLong();
+ assertEquals(fromString(String.valueOf(value)),
UTF8String.fromLong(value));
+ }
+ }
+
+ @Test
+ public void fromBoolean() {
+ assertEquals(fromString("true"), UTF8String.fromBoolean(true));
+ assertEquals(fromString("false"), UTF8String.fromBoolean(false));
+ assertEquals(fromString(String.valueOf(true)),
UTF8String.fromBoolean(true));
+ assertEquals(fromString(String.valueOf(false)),
UTF8String.fromBoolean(false));
Review Comment:
nit: these two lines repeat the assertions above (`String.valueOf(true)` is
`"true"`). Similarly, in `fromLong`, the `boundaries` array overlaps with the
first block and the `10^k` loop. Meanwhile, the documented shared-instance
behavior is not checked. How about asserting it instead, e.g.,
`assertSame(UTF8String.fromBoolean(true), UTF8String.fromBoolean(true))` and
`assertSame(UTF8String.ZERO_UTF8, UTF8String.fromLong(0))`?
##########
common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java:
##########
@@ -194,6 +205,74 @@ public static UTF8String blankString(int length) {
return fromBytes(spaces);
}
+ /**
+ * Creates an UTF8String holding the base-10 representation of a long,
equivalent to
Review Comment:
nit: the Javadoc says this is equivalent to
`fromString(Long.toString(value))`, but `fromLong(0)` returns the shared
`ZERO_UTF8` instance. `fromBoolean` documents its shared-instance behavior, so
could we document it here too, so that callers don't assume they own the
returned buffer?
##########
common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java:
##########
@@ -194,6 +205,74 @@ public static UTF8String blankString(int length) {
return fromBytes(spaces);
}
+ /**
+ * Creates an UTF8String holding the base-10 representation of a long,
equivalent to
+ * `fromString(Long.toString(value))` but without allocating an intermediate
`java.lang.String`.
+ * Callers with narrower integral types (byte, short, int) can widen to long
without changing the
+ * result.
+ */
+ public static UTF8String fromLong(long value) {
+ if (value == 0) {
+ return ZERO_UTF8;
+ }
+ boolean negative = value < 0;
+ // Extract digits in negative space so that Long.MIN_VALUE is
representable (it cannot be
+ // negated). For a negative v and q = v / 100, `q * 100 - v` is the
remainder in [0, 99].
+ long v = negative ? value : -value;
+ byte[] bytes = new byte[digitCount(v) + (negative ? 1 : 0)];
+ int pos = bytes.length;
+ // Write two digits per division: in long arithmetic until the value fits
in an int, then in
+ // cheaper int arithmetic.
+ while (v <= Integer.MIN_VALUE) {
+ long q = v / 100;
+ int r = (int) (q * 100 - v);
+ v = q;
+ bytes[--pos] = DIGIT_PAIRS[2 * r + 1];
+ bytes[--pos] = DIGIT_PAIRS[2 * r];
+ }
+ int iv = (int) v;
+ while (iv <= -100) {
+ int q = iv / 100;
+ int r = q * 100 - iv;
+ iv = q;
+ bytes[--pos] = DIGIT_PAIRS[2 * r + 1];
+ bytes[--pos] = DIGIT_PAIRS[2 * r];
+ }
+ // One or two digits are left, iv is in [-99, -1].
+ if (iv <= -10) {
+ bytes[--pos] = DIGIT_PAIRS[-2 * iv + 1];
+ bytes[--pos] = DIGIT_PAIRS[-2 * iv];
+ } else {
+ bytes[--pos] = (byte) ('0' - iv);
+ }
+ if (negative) {
+ bytes[0] = (byte) '-';
+ }
+ return fromBytes(bytes);
+ }
+
+ /**
+ * Returns the number of decimal digits of a negative long, without dividing.
+ */
+ private static int digitCount(long negativeValue) {
+ long bound = -10;
+ for (int digits = 1; digits < 19; digits++) {
+ if (negativeValue > bound) {
+ return digits;
+ }
+ bound *= 10;
Review Comment:
nit: on the last iteration (`digits == 18`), `bound *= 10` computes
`-10^19`, which overflows and wraps to a positive value. It is harmless today
because the loop exits right after, but it becomes a trap if the loop bound
changes later. Checking before multiplying, or using a static powers-of-ten
table, would avoid it.
--
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]
