You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org> on 2025/02/03 15:30:23 UTC
[PR] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
jonathan-albrecht-ibm opened a new pull request, #49773:
URL: https://github.com/apache/spark/pull/49773
### What changes were proposed in this pull request?
In UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl, when checking if the unused bits in the field are all zeros for fixed length fields, add functions that work on big endian platforms.
### Why are the changes needed?
The current checks only work on little endian platforms. This change detects the platform endianness once when the getPadding* functions are initialized and sets it to the proper implementation for the platform so there should be no significant change in performance.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
This was tested on amd64 (little-endian) and s390x (big-endian) with existing unit tests. On s390x, many existing tests fail in the SQL projects.
### Was this patch authored or co-authored using generative AI tooling?
No
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r3320336429
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtilsSuite.scala:
##########
@@ -157,4 +158,119 @@ class UnsafeRowUtilsSuite extends SparkFunSuite {
StructType(StructField("field",
StructType(StructField("sub", nonBinaryStringType) :: Nil)) :: Nil)))
}
+
+ test("PaddingProvider handles endianness") {
+ // The following arrays contain the same 8 byte field values as represented
+ // in memory on little endian and big endian platforms. They are
+ // what would be seen in memory if the following values were set:
+ //
+ // row.setBoolean(0, true)
+ // row.setByte(1, 0xFF.toByte)
+ // row.setShort(2, 0xFFFE.toShort)
+ // row.setInt(3, 0xFFFEFDFC.toInt)
+ // row.setFloat(4, java.lang.Float.intBitsToFloat(0xFF7FFDFC.toInt))
+ //
+ // Note that in either platform endianness, the values are placed in the first N bytes
+ // of the 8 byte field. The difference is in the order of the bytes in the field.
+ val boolLE = Array[Byte](1, 0, 0, 0, 0, 0, 0, 0)
+ val byteLE = Array[Byte](0xFF.toByte, 0, 0, 0, 0, 0, 0, 0)
+ val shortLE = Array[Byte](0xFE.toByte, 0xFF.toByte, 0, 0, 0, 0, 0, 0)
+ val intLE = Array[Byte](0xFC.toByte, 0xFD.toByte, 0xFE.toByte, 0xFF.toByte, 0, 0, 0, 0)
+ val floatLE = Array[Byte](0xFC.toByte, 0xFD.toByte, 0x7F.toByte, 0xFF.toByte, 0, 0, 0, 0)
+
+ val boolBE = Array[Byte](1, 0, 0, 0, 0, 0, 0, 0)
+ val byteBE = Array[Byte](0xFF.toByte, 0, 0, 0, 0, 0, 0, 0)
+ val shortBE = Array[Byte](0xFF.toByte, 0xFE.toByte, 0, 0, 0, 0, 0, 0)
+ val intBE = Array[Byte](0xFF.toByte, 0xFE.toByte, 0xFD.toByte, 0xFC.toByte, 0, 0, 0, 0)
+ val floatBE = Array[Byte](0xFF.toByte, 0x7F.toByte, 0x7D.toByte, 0xFC.toByte, 0, 0, 0, 0)
Review Comment:
Fixed, thx
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r1983617226
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -74,23 +92,23 @@ object UnsafeRowUtils {
case (field, index) if UnsafeRow.isFixedLength(field.dataType) && !row.isNullAt(index) =>
field.dataType match {
case BooleanType =>
- if ((row.getLong(index) >> 1) != 0L) {
+ if (getPaddingBoolean(row, index) != 0L) {
Review Comment:
Hi @MaxGekk, just wanted to ask if you have some time to look at my comments above
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-3806177463
Hi @dongjoon-hyun,l @MaxGekk I have made the changes that @MaxGekk requested. Could the changes be reviewed?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r1959699669
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -74,23 +92,23 @@ object UnsafeRowUtils {
case (field, index) if UnsafeRow.isFixedLength(field.dataType) && !row.isNullAt(index) =>
field.dataType match {
case BooleanType =>
- if ((row.getLong(index) >> 1) != 0L) {
+ if (getPaddingBoolean(row, index) != 0L) {
Review Comment:
Hi @MaxGekk, just wanted to follow up on this. I hope my explanation makes sense.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4579808614
Thank you, @jonathan-albrecht-ibm and @MaxGekk .
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r3311127479
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -17,11 +17,38 @@
package org.apache.spark.sql.catalyst.util
+import java.nio.ByteOrder.{nativeOrder, BIG_ENDIAN}
+
import org.apache.spark.sql.catalyst.expressions.UnsafeRow
import org.apache.spark.sql.types._
object UnsafeRowUtils {
+ sealed trait PaddingProvider {
+ def getPadding(row: UnsafeRow, index: Int, bitShift: Int): Long
+ def getPaddingBoolean(row: UnsafeRow, index: Int): Long
+ }
+
+ object PaddingProviderLE extends PaddingProvider {
+ override def getPadding(
+ row: UnsafeRow, index: Int, bitShift: Int): Long = row.getLong(index) >> bitShift
+ override def getPaddingBoolean(
+ row: UnsafeRow, index: Int): Long = row.getLong(index) >> 1
+ }
+
+ object PaddingProviderBE extends PaddingProvider {
+ override def getPadding(
+ row: UnsafeRow, index: Int, bitShift: Int): Long = row.getLong(index) << bitShift
+ override def getPaddingBoolean(
+ row: UnsafeRow, index: Int): Long = row.getLong(index) & 0xFEFFFFFFFFFFFFFFL
+ }
+
+ private final val padder: PaddingProvider = if (nativeOrder().equals(BIG_ENDIAN)) {
Review Comment:
Isn't **final** redundant on a val? Also cosmetic:
```suggestion
private final val padder: PaddingProvider = if (nativeOrder() == BIG_ENDIAN)) {
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -17,11 +17,38 @@
package org.apache.spark.sql.catalyst.util
+import java.nio.ByteOrder.{nativeOrder, BIG_ENDIAN}
+
import org.apache.spark.sql.catalyst.expressions.UnsafeRow
import org.apache.spark.sql.types._
object UnsafeRowUtils {
+ sealed trait PaddingProvider {
Review Comment:
Could you make the added trait/objects private (or private[util] if you want them reachable from the suite). They're an internal dispatch detail of one private method.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r2109467725
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -74,23 +92,23 @@ object UnsafeRowUtils {
case (field, index) if UnsafeRow.isFixedLength(field.dataType) && !row.isNullAt(index) =>
field.dataType match {
case BooleanType =>
- if ((row.getLong(index) >> 1) != 0L) {
+ if (getPaddingBoolean(row, index) != 0L) {
Review Comment:
Hi @MaxGekk, just wanted to follow up again to ask if you have some time to look at my comments above
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r1949341193
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -74,23 +92,23 @@ object UnsafeRowUtils {
case (field, index) if UnsafeRow.isFixedLength(field.dataType) && !row.isNullAt(index) =>
field.dataType match {
case BooleanType =>
- if ((row.getLong(index) >> 1) != 0L) {
+ if (getPaddingBoolean(row, index) != 0L) {
Review Comment:
Thanks for the review @MaxGekk!
The `getPadding*` functions are actually created outside of the loop already. They are declared as `val` members and `UnsafeRowUtils` is a top level `object` which makes it a singleton so the `getPadding*` functions are initialized only once for the entire program; not every time they are called. They are initialized with one of the anonymous functions returned by the `if` statement. So when the `getPadding*` functions are called, only the anonymous function chosen at initialization is executed. Only a single instance of the chosen anonymous function is ever created.
It's a really nice concise short form for creating separate classes like `UnsafeRowBeUtils` and `UnsafeRowLeUtils` and choosing one of those to use ahead of time.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r2063938236
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -74,23 +92,23 @@ object UnsafeRowUtils {
case (field, index) if UnsafeRow.isFixedLength(field.dataType) && !row.isNullAt(index) =>
field.dataType match {
case BooleanType =>
- if ((row.getLong(index) >> 1) != 0L) {
+ if (getPaddingBoolean(row, index) != 0L) {
Review Comment:
Hi @MaxGekk, just wanted to follow up again to ask if you have some time to look at my comments above
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r1969684156
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -74,23 +92,23 @@ object UnsafeRowUtils {
case (field, index) if UnsafeRow.isFixedLength(field.dataType) && !row.isNullAt(index) =>
field.dataType match {
case BooleanType =>
- if ((row.getLong(index) >> 1) != 0L) {
+ if (getPaddingBoolean(row, index) != 0L) {
Review Comment:
Hi @MaxGekk, I just wanted to follow up with you on my comments above.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #49773: [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl
URL: https://github.com/apache/spark/pull/49773
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-2642904070
If possible, I'd like to ask for this PR to be backported as far back as branch-3.5 if it is accepted. I have tested this fix on 3.5.4 on big and little endian platforms.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r2239527845
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -74,23 +92,23 @@ object UnsafeRowUtils {
case (field, index) if UnsafeRow.isFixedLength(field.dataType) && !row.isNullAt(index) =>
field.dataType match {
case BooleanType =>
- if ((row.getLong(index) >> 1) != 0L) {
+ if (getPaddingBoolean(row, index) != 0L) {
Review Comment:
Hi @dongjoon-hyun, @MaxGekk, just wondering if anyone has some time to look at my comments above
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4579739399
> Could you retrigger [Run / Build modules: hive - slow tests](https://github.com/linux-on-ibm-z/spark/actions/runs/26640176432/job/78513484356#logs), please.
Thanks, I didn't know I could do that. All tests have passed now
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r2472986884
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -74,23 +92,23 @@ object UnsafeRowUtils {
case (field, index) if UnsafeRow.isFixedLength(field.dataType) && !row.isNullAt(index) =>
field.dataType match {
case BooleanType =>
- if ((row.getLong(index) >> 1) != 0L) {
+ if (getPaddingBoolean(row, index) != 0L) {
Review Comment:
Hi @dongjoon-hyun, @MaxGekk, I have rebased this PR. Just wondering if you have some time to look at my comments above.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4577616858
> @jonathan-albrecht-ibm Could you fix the errors:
>
> ```
> Run ./dev/lint-scala
> Scalastyle checks failed at following occurrences:
> [error] /__w/spark/spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtilsSuite.scala:214: File line length exceeds 100 characters
> [error] /__w/spark/spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtilsSuite.scala:220:8: Public method must have explicit type
> ```
thx @MaxGekk, linter tests are passing now. I think the only failure is unrelated:
[Run / Build modules: hive - slow tests](https://github.com/linux-on-ibm-z/spark/actions/runs/26640176432/job/78513484356#logs)
```
Error: The template is not valid. linux-on-ibm-z/spark/.github/workflows/build_and_test.yml@3a0a23be69f3447473a850138d075948439a16ed (Line: 373, Col: 14): hashFiles('**/pom.xml, project/build.properties, build/mvn, build/sbt, build/sbt-launch-lib.bash, build/spark-build-info') failed. Fail to hash files under directory '/home/runner/work/spark/spark'
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4579323746
Could you retrigger [Run / Build modules: hive - slow tests](https://github.com/linux-on-ibm-z/spark/actions/runs/26640176432/job/78513484356#logs), please.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4567677835
> Spark CI does not run on s390x. A small unit test calling PaddingProviderLE/PaddingProviderBE directly with hand-built Longs (clean + corrupt padding for each of the 5 types) would be feasible on any LE dev machine.
Thanks for reviewing @MaxGekk. I have added unit tests and addressed the review comments. I tried to make the unit tests as simple as possible and also explain how everything works in memory. I hope I succeeded but happy to make any changes :)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r3320300996
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtilsSuite.scala:
##########
@@ -157,4 +158,119 @@ class UnsafeRowUtilsSuite extends SparkFunSuite {
StructType(StructField("field",
StructType(StructField("sub", nonBinaryStringType) :: Nil)) :: Nil)))
}
+
+ test("PaddingProvider handles endianness") {
+ // The following arrays contain the same 8 byte field values as represented
+ // in memory on little endian and big endian platforms. They are
+ // what would be seen in memory if the following values were set:
+ //
+ // row.setBoolean(0, true)
+ // row.setByte(1, 0xFF.toByte)
+ // row.setShort(2, 0xFFFE.toShort)
+ // row.setInt(3, 0xFFFEFDFC.toInt)
+ // row.setFloat(4, java.lang.Float.intBitsToFloat(0xFF7FFDFC.toInt))
+ //
+ // Note that in either platform endianness, the values are placed in the first N bytes
+ // of the 8 byte field. The difference is in the order of the bytes in the field.
+ val boolLE = Array[Byte](1, 0, 0, 0, 0, 0, 0, 0)
+ val byteLE = Array[Byte](0xFF.toByte, 0, 0, 0, 0, 0, 0, 0)
+ val shortLE = Array[Byte](0xFE.toByte, 0xFF.toByte, 0, 0, 0, 0, 0, 0)
+ val intLE = Array[Byte](0xFC.toByte, 0xFD.toByte, 0xFE.toByte, 0xFF.toByte, 0, 0, 0, 0)
+ val floatLE = Array[Byte](0xFC.toByte, 0xFD.toByte, 0x7F.toByte, 0xFF.toByte, 0, 0, 0, 0)
+
+ val boolBE = Array[Byte](1, 0, 0, 0, 0, 0, 0, 0)
+ val byteBE = Array[Byte](0xFF.toByte, 0, 0, 0, 0, 0, 0, 0)
+ val shortBE = Array[Byte](0xFF.toByte, 0xFE.toByte, 0, 0, 0, 0, 0, 0)
+ val intBE = Array[Byte](0xFF.toByte, 0xFE.toByte, 0xFD.toByte, 0xFC.toByte, 0, 0, 0, 0)
+ val floatBE = Array[Byte](0xFF.toByte, 0x7F.toByte, 0x7D.toByte, 0xFC.toByte, 0, 0, 0, 0)
Review Comment:
Probably, a typo in floatBE / floatCorruptBE.
The comment describes the float value as 0xFF7FFDFC. The correct big-endian byte layout is [FF, 7F, FD, FC, ...], but the arrays have [FF, 7F, 7D, FC, ...] - byte index 2 is 0x7D instead of 0xFD.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4579851909
Thanks @MaxGekk and @dongjoon-hyun
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4579800219
+1, LGTM. Merging to master/4.x.
Thank you, @jonathan-albrecht-ibm and @dongjoon-hyun for review.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #49773: [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl
URL: https://github.com/apache/spark/pull/49773
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r3320235709
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -17,11 +17,38 @@
package org.apache.spark.sql.catalyst.util
+import java.nio.ByteOrder.{nativeOrder, BIG_ENDIAN}
+
import org.apache.spark.sql.catalyst.expressions.UnsafeRow
import org.apache.spark.sql.types._
object UnsafeRowUtils {
+ sealed trait PaddingProvider {
Review Comment:
Thanks, done
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4406658973
Hi @dongjoon-hyun, @MaxGekk. Can I get this PR reopened and the Stale tag removed? It is still needed and I don't have permission to reopen it myself.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r1947241109
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -74,23 +92,23 @@ object UnsafeRowUtils {
case (field, index) if UnsafeRow.isFixedLength(field.dataType) && !row.isNullAt(index) =>
field.dataType match {
case BooleanType =>
- if ((row.getLong(index) >> 1) != 0L) {
+ if (getPaddingBoolean(row, index) != 0L) {
Review Comment:
I worry about creating functions in the loop `expectedSchema.fields.zipWithIndex.foreach {`. This might cause perf regression. How about to place `getPadding` and `getPaddingBoolean` in separate classes like `UnsafeRowBeUtils` and `UnsafeRowLeUtils`, and instantiate them out of the loop.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-3487962540
Thanks @dongjoon-hyun, I am refactoring my changes as per @MaxGekk's suggestion https://github.com/apache/spark/pull/49773#discussion_r1947241109
I should have the new changes ready soon
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4402293735
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4554355049
Hi @dongjoon-hyun, @MaxGekk. Can I get this PR reopened and the Stale tag removed? It is still needed and I don't have permission to reopen it myself.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4554593565
Thanks for reopening this PR @MaxGekk. I have made the changes you originally requested if you have time to review
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #49773:
URL: https://github.com/apache/spark/pull/49773#discussion_r3320236449
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtils.scala:
##########
@@ -17,11 +17,38 @@
package org.apache.spark.sql.catalyst.util
+import java.nio.ByteOrder.{nativeOrder, BIG_ENDIAN}
+
import org.apache.spark.sql.catalyst.expressions.UnsafeRow
import org.apache.spark.sql.types._
object UnsafeRowUtils {
+ sealed trait PaddingProvider {
+ def getPadding(row: UnsafeRow, index: Int, bitShift: Int): Long
+ def getPaddingBoolean(row: UnsafeRow, index: Int): Long
+ }
+
+ object PaddingProviderLE extends PaddingProvider {
+ override def getPadding(
+ row: UnsafeRow, index: Int, bitShift: Int): Long = row.getLong(index) >> bitShift
+ override def getPaddingBoolean(
+ row: UnsafeRow, index: Int): Long = row.getLong(index) >> 1
+ }
+
+ object PaddingProviderBE extends PaddingProvider {
+ override def getPadding(
+ row: UnsafeRow, index: Int, bitShift: Int): Long = row.getLong(index) << bitShift
+ override def getPaddingBoolean(
+ row: UnsafeRow, index: Int): Long = row.getLong(index) & 0xFEFFFFFFFFFFFFFFL
+ }
+
+ private final val padder: PaddingProvider = if (nativeOrder().equals(BIG_ENDIAN)) {
Review Comment:
Thanks, done
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4567710830
lgtm except of a comment.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51069][SQL] Add big-endian support to UnsafeRowUtils.validateStructuralIntegrityWithReasonImpl [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #49773:
URL: https://github.com/apache/spark/pull/49773#issuecomment-4571428403
@jonathan-albrecht-ibm Could you fix the errors:
```
Run ./dev/lint-scala
Scalastyle checks failed at following occurrences:
[error] /__w/spark/spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtilsSuite.scala:214: File line length exceeds 100 characters
[error] /__w/spark/spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/UnsafeRowUtilsSuite.scala:220:8: Public method must have explicit type
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org