You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2023/08/10 16:50:01 UTC
[GitHub] [spark] andygrove opened a new pull request, #42434: [WIP][SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
andygrove opened a new pull request, #42434:
URL: https://github.com/apache/spark/pull/42434
WIP
--
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
[GitHub] [spark] tgravescs commented on pull request #42434: [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "tgravescs (via GitHub)" <gi...@apache.org>.
tgravescs commented on PR #42434:
URL: https://github.com/apache/spark/pull/42434#issuecomment-1677320448
+1. @cloud-fan, this ok with you 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
[GitHub] [spark] cloud-fan closed pull request #42434: [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #42434: [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
URL: https://github.com/apache/spark/pull/42434
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #42434: [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42434:
URL: https://github.com/apache/spark/pull/42434#discussion_r1290790074
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala:
##########
@@ -80,11 +80,16 @@ trait ExpressionEvalHelper extends ScalaCheckDrivenPropertyChecks with PlanTestB
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow,
+ withCodegen: Boolean = false): Unit = {
// Make it as method to obtain fresh expression everytime.
def expr = prepareEvaluation(expression)
val catalystValue = CatalystTypeConverters.convertToCatalyst(expected)
- checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)
+ if (withCodegen) {
+ checkEvaluation(expr, catalystValue, inputRow)
+ } else {
+ checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)
+ }
checkEvaluationWithMutableProjection(expr, catalystValue, inputRow)
if (GenerateUnsafeProjection.canSupport(expr.dataType)) {
checkEvaluationWithUnsafeProjection(expr, catalystValue, inputRow)
Review Comment:
The test already checked codegen before. We don't need to touch this file, just update the test to put null in the first element.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala:
##########
@@ -80,11 +80,16 @@ trait ExpressionEvalHelper extends ScalaCheckDrivenPropertyChecks with PlanTestB
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow,
+ withCodegen: Boolean = false): Unit = {
// Make it as method to obtain fresh expression everytime.
def expr = prepareEvaluation(expression)
val catalystValue = CatalystTypeConverters.convertToCatalyst(expected)
- checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)
+ if (withCodegen) {
+ checkEvaluation(expr, catalystValue, inputRow)
+ } else {
+ checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)
+ }
checkEvaluationWithMutableProjection(expr, catalystValue, inputRow)
if (GenerateUnsafeProjection.canSupport(expr.dataType)) {
checkEvaluationWithUnsafeProjection(expr, catalystValue, inputRow)
Review Comment:
The test already checks codegen. We don't need to touch this file, just update the test to put null in the first element.
--
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
[GitHub] [spark] cloud-fan commented on pull request #42434: [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #42434:
URL: https://github.com/apache/spark/pull/42434#issuecomment-1678331752
thanks, merging to master/3.5!
--
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
[GitHub] [spark] tgravescs commented on a diff in pull request #42434: [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "tgravescs (via GitHub)" <gi...@apache.org>.
tgravescs commented on code in PR #42434:
URL: https://github.com/apache/spark/pull/42434#discussion_r1290671657
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala:
##########
@@ -80,11 +80,15 @@ trait ExpressionEvalHelper extends ScalaCheckDrivenPropertyChecks with PlanTestB
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow,
+ withCodegen: Boolean = false): Unit = {
// Make it as method to obtain fresh expression everytime.
def expr = prepareEvaluation(expression)
val catalystValue = CatalystTypeConverters.convertToCatalyst(expected)
checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)
+ if (withCodegen) {
Review Comment:
we should document that withCodegen will evaluate it twice, once without and once with.
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #42434: [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42434:
URL: https://github.com/apache/spark/pull/42434#discussion_r1293486211
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala:
##########
@@ -826,9 +826,9 @@ abstract class CastSuiteBase extends SparkFunSuite with ExpressionEvalHelper {
val ret1 = cast(Literal.create(Map(1 -> "a", 2 -> "b", 3 -> "c")), StringType)
checkEvaluation(ret1, s"${lb}1 -> a, 2 -> b, 3 -> c$rb")
val ret2 = cast(
- Literal.create(Map("1" -> "a".getBytes, "2" -> null, "3" -> "c".getBytes)),
+ Literal.create(Map("1" -> null, "2" -> "a".getBytes, "3" -> "c".getBytes)),
Review Comment:
can we also have a null value in the middle? so that we can have test coverage for null in different positions.
--
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
[GitHub] [spark] andygrove commented on a diff in pull request #42434: [WIP][SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #42434:
URL: https://github.com/apache/spark/pull/42434#discussion_r1290421968
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToStringBase.scala:
##########
@@ -352,7 +352,7 @@ trait ToStringBase { self: UnaryExpression with TimeZoneAwareExpression =>
| $buffer.append($keyToStringFunc($getMapFirstKey));
| $buffer.append(" ->");
| if ($map.valueArray().isNullAt(0)) {
- | ${appendNull(buffer, isFirstElement = true)}
+ | ${appendNull(buffer, isFirstElement = false)}
Review Comment:
There should always be a space after `->` and before the value. It does not matter if this is the first key-value pair or not.
--
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
[GitHub] [spark] andygrove closed pull request #42434: [WIP][SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove closed pull request #42434: [WIP][SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
URL: https://github.com/apache/spark/pull/42434
--
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
[GitHub] [spark] andygrove commented on a diff in pull request #42434: [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #42434:
URL: https://github.com/apache/spark/pull/42434#discussion_r1293563603
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala:
##########
@@ -826,9 +826,9 @@ abstract class CastSuiteBase extends SparkFunSuite with ExpressionEvalHelper {
val ret1 = cast(Literal.create(Map(1 -> "a", 2 -> "b", 3 -> "c")), StringType)
checkEvaluation(ret1, s"${lb}1 -> a, 2 -> b, 3 -> c$rb")
val ret2 = cast(
- Literal.create(Map("1" -> "a".getBytes, "2" -> null, "3" -> "c".getBytes)),
+ Literal.create(Map("1" -> null, "2" -> "a".getBytes, "3" -> "c".getBytes)),
Review Comment:
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
[GitHub] [spark] andygrove commented on a diff in pull request #42434: [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #42434:
URL: https://github.com/apache/spark/pull/42434#discussion_r1291414158
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala:
##########
@@ -80,11 +80,16 @@ trait ExpressionEvalHelper extends ScalaCheckDrivenPropertyChecks with PlanTestB
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow,
+ withCodegen: Boolean = false): Unit = {
// Make it as method to obtain fresh expression everytime.
def expr = prepareEvaluation(expression)
val catalystValue = CatalystTypeConverters.convertToCatalyst(expected)
- checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)
+ if (withCodegen) {
+ checkEvaluation(expr, catalystValue, inputRow)
+ } else {
+ checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)
+ }
checkEvaluationWithMutableProjection(expr, catalystValue, inputRow)
if (GenerateUnsafeProjection.canSupport(expr.dataType)) {
checkEvaluationWithUnsafeProjection(expr, catalystValue, inputRow)
Review Comment:
Thanks. I reverted that change and confirmed that the test still fails if I remove the fix.
--
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
[GitHub] [spark] andygrove commented on a diff in pull request #42434: [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #42434:
URL: https://github.com/apache/spark/pull/42434#discussion_r1290716061
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala:
##########
@@ -80,11 +80,15 @@ trait ExpressionEvalHelper extends ScalaCheckDrivenPropertyChecks with PlanTestB
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow,
+ withCodegen: Boolean = false): Unit = {
// Make it as method to obtain fresh expression everytime.
def expr = prepareEvaluation(expression)
val catalystValue = CatalystTypeConverters.convertToCatalyst(expected)
checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)
+ if (withCodegen) {
Review Comment:
I updated the code so that it only evaluates once, either with or without codegen
--
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
[GitHub] [spark] yaooqinn commented on pull request #42434: [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #42434:
URL: https://github.com/apache/spark/pull/42434#issuecomment-1678389749
Late LGTM
--
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