You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2024/10/17 02:57:40 UTC
[PR] [SPARK-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
panbingkun opened a new pull request, #48509:
URL: https://github.com/apache/spark/pull/48509
### What changes were proposed in this pull request?
The pr aims to use `Invoke` to implement `JsonToStructs`(`from_json`).
### Why are the changes needed?
Based on @cloud-fan's suggestion, I believe that implementing `JsonToStructs`(`from_json`) with `Invoke` can greatly simplify the code.
https://github.com/apache/spark/pull/48466#discussion_r1802533505
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Update existed UT.
### 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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #48509: [SPARK-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`)
URL: https://github.com/apache/spark/pull/48509
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1859630198
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJsonExprsSuite.scala:
##########
@@ -292,7 +292,7 @@ class OptimizeJsonExprsSuite extends PlanTest with ExpressionEvalHelper {
Seq("""{"a":1, "b":2, "c": 123, "d": "test"}""", null).foreach(v => {
val row = create_row(v)
- checkEvaluation(e1, e2.eval(row), row)
+ checkEvaluation(e1, replace(e2).eval(row), row)
Review Comment:
Or would it break this rule if we `manually` implement it instead of using `Invoke`?
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1859607312
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJsonExprsSuite.scala:
##########
@@ -292,7 +292,7 @@ class OptimizeJsonExprsSuite extends PlanTest with ExpressionEvalHelper {
Seq("""{"a":1, "b":2, "c": 123, "d": "test"}""", null).foreach(v => {
val row = create_row(v)
- checkEvaluation(e1, e2.eval(row), row)
+ checkEvaluation(e1, replace(e2).eval(row), row)
Review Comment:
is this one - `OptimizeCsvJsonExprs`?
https://github.com/apache/spark/blob/e55511c98e14115dd023f9014372fc7f23d9ef6c/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeCsvJsonExprs.scala#L39
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804323150
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/json/JsonExpressionEvalUtils.scala:
##########
@@ -58,11 +59,11 @@ object JsonExpressionEvalUtils {
}
class JsonToStructsEvaluator(
Review Comment:
shall we make it a case class?
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804043853
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala:
##########
@@ -322,21 +322,22 @@ case class StaticInvoke(
val evaluate = if (returnNullable && !method.getReturnType.isPrimitive) {
if (CodeGenerator.defaultValue(dataType) == "null") {
s"""
- ${ev.value} = $callFunc;
+ ${ev.value} = ($javaType) $callFunc;
Review Comment:
- Because some methods called by `StaticInvoke` may return a `Any`(or `Object`) value, eg:
https://github.com/apache/spark/blob/070f2bdfb968c8080de1c6614c1def978df823d4/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/json/JsonExpressionEvalUtils.scala#L103-L112
- Based on the `dataType`, obtain the `Java type` and then `cast`.
- I think this pattern can greatly simplify the methods called by `StaticInvoke` (without writing `multiple` methods)
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1859607312
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJsonExprsSuite.scala:
##########
@@ -292,7 +292,7 @@ class OptimizeJsonExprsSuite extends PlanTest with ExpressionEvalHelper {
Seq("""{"a":1, "b":2, "c": 123, "d": "test"}""", null).foreach(v => {
val row = create_row(v)
- checkEvaluation(e1, e2.eval(row), row)
+ checkEvaluation(e1, replace(e2).eval(row), row)
Review Comment:
is this one? `OptimizeCsvJsonExprs`?
https://github.com/apache/spark/blob/e55511c98e14115dd023f9014372fc7f23d9ef6c/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeCsvJsonExprs.scala#L39
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #48509:
URL: https://github.com/apache/spark/pull/48509#issuecomment-2422449183
+1, LGTM. Merging to master.
Thank you, @panbingkun and @cloud-fan 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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804361635
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -718,6 +692,21 @@ case class JsonToStructs(
override def prettyName: String = "from_json"
+ @transient
+ private val nameOfCorruptRecord = SQLConf.get.getConf(SQLConf.COLUMN_NAME_OF_CORRUPT_RECORD)
+
+ @transient
+ lazy val evaluator = new JsonToStructsEvaluator(
+ options, nullableSchema, nameOfCorruptRecord, timeZoneId, variantAllowDuplicateKeys)
+
+ override def replacement: Expression = Invoke(
Review Comment:
Sure, allow me to do 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: 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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804324876
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -718,6 +692,21 @@ case class JsonToStructs(
override def prettyName: String = "from_json"
+ @transient
+ private val nameOfCorruptRecord = SQLConf.get.getConf(SQLConf.COLUMN_NAME_OF_CORRUPT_RECORD)
+
+ @transient
+ lazy val evaluator = new JsonToStructsEvaluator(
+ options, nullableSchema, nameOfCorruptRecord, timeZoneId, variantAllowDuplicateKeys)
+
+ override def replacement: Expression = Invoke(
Review Comment:
There were some fixes to `StaticInvoke` in this PR, `Invoke` doesn't have these issues?
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1858729232
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJsonExprsSuite.scala:
##########
@@ -292,7 +292,7 @@ class OptimizeJsonExprsSuite extends PlanTest with ExpressionEvalHelper {
Seq("""{"a":1, "b":2, "c": 123, "d": "test"}""", null).foreach(v => {
val row = create_row(v)
- checkEvaluation(e1, e2.eval(row), row)
+ checkEvaluation(e1, replace(e2).eval(row), row)
Review Comment:
also cc @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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1859597648
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJsonExprsSuite.scala:
##########
@@ -292,7 +292,7 @@ class OptimizeJsonExprsSuite extends PlanTest with ExpressionEvalHelper {
Seq("""{"a":1, "b":2, "c": 123, "d": "test"}""", null).foreach(v => {
val row = create_row(v)
- checkEvaluation(e1, e2.eval(row), row)
+ checkEvaluation(e1, replace(e2).eval(row), row)
Review Comment:
Which optimization rule or scenario is not effective?
I can investigate it.
Thanks!
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804043853
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala:
##########
@@ -322,21 +322,22 @@ case class StaticInvoke(
val evaluate = if (returnNullable && !method.getReturnType.isPrimitive) {
if (CodeGenerator.defaultValue(dataType) == "null") {
s"""
- ${ev.value} = $callFunc;
+ ${ev.value} = ($javaType) $callFunc;
Review Comment:
- Because some methods called by `StaticInvoke` may return a `Any`(or `Object`) value, eg:
https://github.com/apache/spark/blob/070f2bdfb968c8080de1c6614c1def978df823d4/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/json/JsonExpressionEvalUtils.scala#L103-L112
- Based on the `dataType`, obtain the `Java type` and then `cast`.
- I think this pattern can greatly simplify the methods called by `StaticInvoke` (without writing `multiple` methods)
- Otherwise, the compilation will fail, as follows:
<img width="1234" alt="image" src="https://github.com/user-attachments/assets/8d42a3ca-b9f4-46a0-954a-d87957806d54">
```java
/* 049 */ boolean project_isNull_0 = project_resultIsNull_0;
/* 050 */ InternalRow project_value_0 = null;
/* 051 */ if (!project_resultIsNull_0) {
/* 052 */ project_value_0 = org.apache.spark.sql.catalyst.expressions.json.JsonExpressionEvalUtils.fromJson(project_mutableStateArray_0[0], project_mutableStateArray_1[0]);
/* 053 */ project_isNull_0 = project_value_0 == null;
/* 054 */
/* 055 */ }
```
```shell
11:15:52.920 ERROR org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator: Failed to compile the generated Java code.
org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 52, Column 1: Assignment conversion not possible from type "java.lang.Object" to type "org.apache.spark.sql.catalyst.InternalRow"
at org.codehaus.janino.UnitCompiler.compileError(UnitCompiler.java:13014)
at org.codehaus.janino.UnitCompiler.assignmentConversion(UnitCompiler.java:11263)
at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:4099)
at org.codehaus.janino.UnitCompiler.access$6300(UnitCompiler.java:236)
at org.codehaus.janino.UnitCompiler$13.visitAssignment(UnitCompiler.java:4057)
at org.codehaus.janino.UnitCompiler$13.visitAssignment(UnitCompiler.java:4040)
at org.codehaus.janino.Java$Assignment.accept(Java.java:4864)
```
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804339739
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -718,6 +692,21 @@ case class JsonToStructs(
override def prettyName: String = "from_json"
+ @transient
+ private val nameOfCorruptRecord = SQLConf.get.getConf(SQLConf.COLUMN_NAME_OF_CORRUPT_RECORD)
+
+ @transient
+ lazy val evaluator = new JsonToStructsEvaluator(
+ options, nullableSchema, nameOfCorruptRecord, timeZoneId, variantAllowDuplicateKeys)
+
+ override def replacement: Expression = Invoke(
Review Comment:
Yes, in `Invoke`, `cast` is added by default to the results of method calls, as shown below
- Invoke
https://github.com/apache/spark/blob/2469a6f29c0e6f59e928f4f4e7aa9709768d4757/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala#L477
https://github.com/apache/spark/blob/2469a6f29c0e6f59e928f4f4e7aa9709768d4757/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala#L481
- StaticInvoke (while `StaticInvoke` does not)
https://github.com/apache/spark/blob/2469a6f29c0e6f59e928f4f4e7aa9709768d4757/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala#L325
https://github.com/apache/spark/blob/2469a6f29c0e6f59e928f4f4e7aa9709768d4757/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala#L331
https://github.com/apache/spark/blob/2469a6f29c0e6f59e928f4f4e7aa9709768d4757/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala#L345
- I think it's also necessary to add `cast` on `StaticInvoke`. we haven't encountered any similar cases so far. (At least this case was bypassed using `Invoke`)
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804360328
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/json/JsonExpressionEvalUtils.scala:
##########
@@ -58,11 +59,11 @@ object JsonExpressionEvalUtils {
}
class JsonToStructsEvaluator(
Review Comment:
Yeah, this idea is `great` and can `simplify` code.
Thank you very much!
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804305720
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/json/JsonExpressionEvalUtils.scala:
##########
@@ -55,6 +55,10 @@ object JsonExpressionEvalUtils {
UTF8String.fromString(dt.sql)
}
+
+ def fromJson(jsonToStructsEvaluator: JsonToStructsEvaluator, json: UTF8String): Any = {
Review Comment:
Updated.
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804354706
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -718,6 +692,21 @@ case class JsonToStructs(
override def prettyName: String = "from_json"
+ @transient
+ private val nameOfCorruptRecord = SQLConf.get.getConf(SQLConf.COLUMN_NAME_OF_CORRUPT_RECORD)
+
+ @transient
+ lazy val evaluator = new JsonToStructsEvaluator(
+ options, nullableSchema, nameOfCorruptRecord, timeZoneId, variantAllowDuplicateKeys)
+
+ override def replacement: Expression = Invoke(
Review Comment:
ah, can we fix `StaticInvoke` in a separated PR and write a unit test for 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: 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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1859718089
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJsonExprsSuite.scala:
##########
@@ -292,7 +292,7 @@ class OptimizeJsonExprsSuite extends PlanTest with ExpressionEvalHelper {
Seq("""{"a":1, "b":2, "c": 123, "d": "test"}""", null).foreach(v => {
val row = create_row(v)
- checkEvaluation(e1, e2.eval(row), row)
+ checkEvaluation(e1, replace(e2).eval(row), row)
Review Comment:
Okay, let me revert it first and keep it implemented manually.
Also, `CsvToStructs` may also have this issue, right?
Let's manually implement its 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
Re: [PR] [SPARK-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1858515472
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJsonExprsSuite.scala:
##########
@@ -292,7 +292,7 @@ class OptimizeJsonExprsSuite extends PlanTest with ExpressionEvalHelper {
Seq("""{"a":1, "b":2, "c": 123, "d": "test"}""", null).foreach(v => {
val row = create_row(v)
- checkEvaluation(e1, e2.eval(row), row)
+ checkEvaluation(e1, replace(e2).eval(row), row)
Review Comment:
I missed this. `JsonToStructs` needs to carry the semantic information so that the optimizer rule can match it and do something. Replacing it with `Invoke` breaks it.
I propose to revert it and revisit other PRs that reimplement expressions with `RuntimeReplaceable`.
cc @panbingkun
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1859818783
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJsonExprsSuite.scala:
##########
@@ -292,7 +292,7 @@ class OptimizeJsonExprsSuite extends PlanTest with ExpressionEvalHelper {
Seq("""{"a":1, "b":2, "c": 123, "d": "test"}""", null).foreach(v => {
val row = create_row(v)
- checkEvaluation(e1, e2.eval(row), row)
+ checkEvaluation(e1, replace(e2).eval(row), row)
Review Comment:
Yea let's manually implement codegen first.
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804099446
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/json/JsonExpressionEvalUtils.scala:
##########
@@ -55,6 +55,10 @@ object JsonExpressionEvalUtils {
UTF8String.fromString(dt.sql)
}
+
+ def fromJson(jsonToStructsEvaluator: JsonToStructsEvaluator, json: UTF8String): Any = {
Review Comment:
why do we prefer `StaticInvoke` over `Invoke`? is there a feature gap?
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804165591
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/json/JsonExpressionEvalUtils.scala:
##########
@@ -55,6 +55,10 @@ object JsonExpressionEvalUtils {
UTF8String.fromString(dt.sql)
}
+
+ def fromJson(jsonToStructsEvaluator: JsonToStructsEvaluator, json: UTF8String): Any = {
Review Comment:
Just a personal preference, let me use `Invoke` to implement 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: 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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1804355396
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -718,6 +692,21 @@ case class JsonToStructs(
override def prettyName: String = "from_json"
+ @transient
+ private val nameOfCorruptRecord = SQLConf.get.getConf(SQLConf.COLUMN_NAME_OF_CORRUPT_RECORD)
+
+ @transient
+ lazy val evaluator = new JsonToStructsEvaluator(
+ options, nullableSchema, nameOfCorruptRecord, timeZoneId, variantAllowDuplicateKeys)
+
+ override def replacement: Expression = Invoke(
Review Comment:
We conducted a GA test on `StaticInvoke` after adding `cast`, and GA is Okay, which can cover all current scenarios.
https://github.com/apache/spark/pull/48507/files
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1859687536
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJsonExprsSuite.scala:
##########
@@ -292,7 +292,7 @@ class OptimizeJsonExprsSuite extends PlanTest with ExpressionEvalHelper {
Seq("""{"a":1, "b":2, "c": 123, "d": "test"}""", null).foreach(v => {
val row = create_row(v)
- checkEvaluation(e1, e2.eval(row), row)
+ checkEvaluation(e1, replace(e2).eval(row), row)
Review Comment:
The problem is, it's not only about matching, but also about replacing. In `OptimizeCsvJsonExprs`, we need to replace the `JsonToStructs` with a new one using a different schema, and it's very hard to do so with matching `Invoke`.
In the long term, I think we should make `RuntimeReplaceable` really do replacement at runtime, instead of at the beginning of the optimizer, especially for the one with `StaticInvoke`/`Invoke`.
--
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-49966][SQL] Use `Invoke` to implement `JsonToStructs`(`from_json`) [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48509:
URL: https://github.com/apache/spark/pull/48509#discussion_r1858776934
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJsonExprsSuite.scala:
##########
@@ -292,7 +292,7 @@ class OptimizeJsonExprsSuite extends PlanTest with ExpressionEvalHelper {
Seq("""{"a":1, "b":2, "c": 123, "d": "test"}""", null).foreach(v => {
val row = create_row(v)
- checkEvaluation(e1, e2.eval(row), row)
+ checkEvaluation(e1, replace(e2).eval(row), row)
Review Comment:
Do you mean we matched `JsonToStructs` somewhere, but after the changes the optimization rules don't work anymore?
--
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