You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2025/01/17 12:43:59 UTC
[PR] [SPARK-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
beliefer opened a new pull request, #49555:
URL: https://github.com/apache/spark/pull/49555
### What changes were proposed in this pull request?
This PR proposes to improve the push down information for binary.
### Why are the changes needed?
Before this PR. the push down information for binary looks like below.
`PushedFilters: [binary_col IS NOT NULL, binary_col = [B@6e5af973]`
After this PR, the push down information will be:
`PushedFilters: [binary_col IS NOT NULL, binary_col = X'123456']`
### Does this PR introduce _any_ user-facing change?
'Yes'.
Fix the bug.
### How was this patch tested?
GA
### 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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #49555:
URL: https://github.com/apache/spark/pull/49555#discussion_r1921668160
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -388,12 +388,13 @@ private[sql] object HoursTransform {
}
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
- override def toString: String = {
- if (dataType.isInstanceOf[StringType]) {
- s"'${StringUtils.replace(s"$value", "'", "''")}'"
- } else {
- s"$value"
- }
+ override def toString: String = dataType match {
+ case StringType => s"'${StringUtils.replace(s"$value", "'", "''")}'"
+ case BinaryType =>
Review Comment:
From a cursory look, it seems so but would be great to check if this complies ANSI, and mention it in PR 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: 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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #49555:
URL: https://github.com/apache/spark/pull/49555#issuecomment-2598302725
ping @cloud-fan cc @sunxiaoguang
--
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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #49555:
URL: https://github.com/apache/spark/pull/49555#discussion_r1921669146
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -388,12 +388,13 @@ private[sql] object HoursTransform {
}
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
- override def toString: String = {
- if (dataType.isInstanceOf[StringType]) {
- s"'${StringUtils.replace(s"$value", "'", "''")}'"
- } else {
- s"$value"
- }
+ override def toString: String = dataType match {
+ case StringType => s"'${StringUtils.replace(s"$value", "'", "''")}'"
+ case BinaryType =>
+ assert(value.isInstanceOf[Array[Byte]])
+ val bytes = value.asInstanceOf[Array[Byte]]
+ bytes.map("%02X".format(_)).mkString("X'", "", "'")
Review Comment:
Seems like `X` literal isn't universal and depends on dialect (https://github.com/apache/spark/pull/49452)
--
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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #49555:
URL: https://github.com/apache/spark/pull/49555#discussion_r1923350962
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -388,12 +388,13 @@ private[sql] object HoursTransform {
}
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
- override def toString: String = {
- if (dataType.isInstanceOf[StringType]) {
- s"'${StringUtils.replace(s"$value", "'", "''")}'"
- } else {
- s"$value"
- }
+ override def toString: String = dataType match {
+ case StringType => s"'${StringUtils.replace(s"$value", "'", "''")}'"
+ case BinaryType =>
+ assert(value.isInstanceOf[Array[Byte]])
+ val bytes = value.asInstanceOf[Array[Byte]]
+ bytes.map("%02X".format(_)).mkString("X'", "", "'")
Review Comment:
where else do we use `encodeHexString`?
--
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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #49555:
URL: https://github.com/apache/spark/pull/49555#discussion_r1921843259
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -388,12 +388,13 @@ private[sql] object HoursTransform {
}
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
- override def toString: String = {
- if (dataType.isInstanceOf[StringType]) {
- s"'${StringUtils.replace(s"$value", "'", "''")}'"
- } else {
- s"$value"
- }
+ override def toString: String = dataType match {
+ case StringType => s"'${StringUtils.replace(s"$value", "'", "''")}'"
+ case BinaryType =>
+ assert(value.isInstanceOf[Array[Byte]])
+ val bytes = value.asInstanceOf[Array[Byte]]
+ bytes.map("%02X".format(_)).mkString("X'", "", "'")
Review Comment:
Yes. you said right. The output information is just a generic represent like the others.
--
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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #49555:
URL: https://github.com/apache/spark/pull/49555#discussion_r1923352440
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -388,12 +388,13 @@ private[sql] object HoursTransform {
}
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
- override def toString: String = {
- if (dataType.isInstanceOf[StringType]) {
- s"'${StringUtils.replace(s"$value", "'", "''")}'"
- } else {
- s"$value"
- }
+ override def toString: String = dataType match {
+ case StringType => s"'${StringUtils.replace(s"$value", "'", "''")}'"
+ case BinaryType =>
+ assert(value.isInstanceOf[Array[Byte]])
+ val bytes = value.asInstanceOf[Array[Byte]]
+ bytes.map("%02X".format(_)).mkString("X'", "", "'")
Review Comment:
Anyway, this is for display only, we just need a popular binary value pretty string format.
--
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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #49555:
URL: https://github.com/apache/spark/pull/49555#discussion_r1923797461
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -388,12 +388,13 @@ private[sql] object HoursTransform {
}
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
- override def toString: String = {
- if (dataType.isInstanceOf[StringType]) {
- s"'${StringUtils.replace(s"$value", "'", "''")}'"
- } else {
- s"$value"
- }
+ override def toString: String = dataType match {
+ case StringType => s"'${StringUtils.replace(s"$value", "'", "''")}'"
+ case BinaryType =>
+ assert(value.isInstanceOf[Array[Byte]])
+ val bytes = value.asInstanceOf[Array[Byte]]
+ bytes.map("%02X".format(_)).mkString("X'", "", "'")
Review Comment:
Spark SQL already parsed the format `X'123456'`.
The binary literal show the format `0x123456`.
```
override def toString: String = value match {
case null => "null"
case binary: Array[Byte] => "0x" + ApacheHex.encodeHexString(binary, false)
```
Which one is the better?
--
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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #49555:
URL: https://github.com/apache/spark/pull/49555#issuecomment-2612671387
@MaxGekk @cloud-fan @HyukjinKwon Thank you for all!
--
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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #49555:
URL: https://github.com/apache/spark/pull/49555#discussion_r1923797461
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -388,12 +388,13 @@ private[sql] object HoursTransform {
}
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
- override def toString: String = {
- if (dataType.isInstanceOf[StringType]) {
- s"'${StringUtils.replace(s"$value", "'", "''")}'"
- } else {
- s"$value"
- }
+ override def toString: String = dataType match {
+ case StringType => s"'${StringUtils.replace(s"$value", "'", "''")}'"
+ case BinaryType =>
+ assert(value.isInstanceOf[Array[Byte]])
+ val bytes = value.asInstanceOf[Array[Byte]]
+ bytes.map("%02X".format(_)).mkString("X'", "", "'")
Review Comment:
Spark SQL already parsed the format `X'123456'`.
The binary literal show the format `0x123456`.
```
override def toString: String = value match {
case null => "null"
case binary: Array[Byte] => "0x" + ApacheHex.encodeHexString(binary, false)
```
Which is the better one?
--
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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #49555:
URL: https://github.com/apache/spark/pull/49555#discussion_r1921668160
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -388,12 +388,13 @@ private[sql] object HoursTransform {
}
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
- override def toString: String = {
- if (dataType.isInstanceOf[StringType]) {
- s"'${StringUtils.replace(s"$value", "'", "''")}'"
- } else {
- s"$value"
- }
+ override def toString: String = dataType match {
+ case StringType => s"'${StringUtils.replace(s"$value", "'", "''")}'"
+ case BinaryType =>
Review Comment:
From a cursory look, it seems so but would be great to check if this complies ANSI, and mention it in PR 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: 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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #49555:
URL: https://github.com/apache/spark/pull/49555#discussion_r1923797461
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -388,12 +388,13 @@ private[sql] object HoursTransform {
}
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
- override def toString: String = {
- if (dataType.isInstanceOf[StringType]) {
- s"'${StringUtils.replace(s"$value", "'", "''")}'"
- } else {
- s"$value"
- }
+ override def toString: String = dataType match {
+ case StringType => s"'${StringUtils.replace(s"$value", "'", "''")}'"
+ case BinaryType =>
+ assert(value.isInstanceOf[Array[Byte]])
+ val bytes = value.asInstanceOf[Array[Byte]]
+ bytes.map("%02X".format(_)).mkString("X'", "", "'")
Review Comment:
Spark SQL already parsed the format `X'123456'`.
The binary literal show the format `0x123456`.
```
override def toString: String = value match {
case null => "null"
case binary: Array[Byte] => "0x" + ApacheHex.encodeHexString(binary, false)
```
Which is better?
--
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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #49555:
URL: https://github.com/apache/spark/pull/49555#discussion_r1923797461
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -388,12 +388,13 @@ private[sql] object HoursTransform {
}
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
- override def toString: String = {
- if (dataType.isInstanceOf[StringType]) {
- s"'${StringUtils.replace(s"$value", "'", "''")}'"
- } else {
- s"$value"
- }
+ override def toString: String = dataType match {
+ case StringType => s"'${StringUtils.replace(s"$value", "'", "''")}'"
+ case BinaryType =>
+ assert(value.isInstanceOf[Array[Byte]])
+ val bytes = value.asInstanceOf[Array[Byte]]
+ bytes.map("%02X".format(_)).mkString("X'", "", "'")
Review Comment:
Spark SQL already parsed the format `X'123456'`.
The binary literal show the format `0x123456`.
```
override def toString: String = value match {
case null => "null"
case binary: Array[Byte] => "0x" + ApacheHex.encodeHexString(binary, false)
```
Which is better?
--
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-50792][SQL][FOLLOWUP] Improve the push down information for binary [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #49555:
URL: https://github.com/apache/spark/pull/49555#discussion_r1923216437
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -388,12 +388,13 @@ private[sql] object HoursTransform {
}
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
- override def toString: String = {
- if (dataType.isInstanceOf[StringType]) {
- s"'${StringUtils.replace(s"$value", "'", "''")}'"
- } else {
- s"$value"
- }
+ override def toString: String = dataType match {
+ case StringType => s"'${StringUtils.replace(s"$value", "'", "''")}'"
+ case BinaryType =>
+ assert(value.isInstanceOf[Array[Byte]])
+ val bytes = value.asInstanceOf[Array[Byte]]
+ bytes.map("%02X".format(_)).mkString("X'", "", "'")
Review Comment:
How about to use `ApacheHex.encodeHexString(binary, ...` as in other places in the file.
--
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