You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/12/30 13:52:43 UTC
[GitHub] [spark] mattshma opened a new pull request, #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
mattshma opened a new pull request, #39315:
URL: https://github.com/apache/spark/pull/39315
### What changes were proposed in this pull request?
We'll get wrong data when transform only specify reader or writer 's row format delimited, the reason is using the wrong format to feed/fetch data to/from running script now. we should set the format correctly.
### Why are the changes needed?
Fix transform writer format and reader format.
### Does this PR introduce _any_ user-facing change?
When we set transform's row format delimited in the sql, we may get the wrong data.
### How was this patch tested?
New tests.
--
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] HyukjinKwon closed pull request #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
URL: https://github.com/apache/spark/pull/39315
--
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] AngersZhuuuu commented on a diff in pull request #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #39315:
URL: https://github.com/apache/spark/pull/39315#discussion_r1060431759
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala:
##########
@@ -751,15 +751,15 @@ class SparkSqlAstBuilder extends AstBuilder {
(Nil, Option(name), props, recordHandler)
}
- val (inFormat, inSerdeClass, inSerdeProps, reader) =
+ val (inFormat, inSerdeClass, inSerdeProps, writer) =
Review Comment:
Maybe we can add some comments to show why we write code like this.
--
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] AngersZhuuuu commented on a diff in pull request #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #39315:
URL: https://github.com/apache/spark/pull/39315#discussion_r1060255373
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/BaseScriptTransformationSuite.scala:
##########
@@ -670,6 +670,26 @@ abstract class BaseScriptTransformationSuite extends SparkPlanTest with SQLTestU
),
df.select($"col").collect())
}
+
+ test("SPARK-41790: Set TRANSFORM reader and writer's format correctly") {
+ withTempView("v") {
+ val df = Seq(
+ (1, 2)
+ ).toDF("a", "b")
+ df.createTempView("v")
+
+ checkAnswer(
+ sql(
+ s"""
+ |SELECT TRANSFORM(a, b)
+ | ROW FORMAT DELIMITED
+ | FIELDS TERMINATED BY ','
+ | USING 'cat'
+ | AS (c)
+ |FROM v
+ """.stripMargin), identity, Row("1,2") :: Nil)
Review Comment:
Can you explain why this UT can verify the change?
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala:
##########
@@ -751,14 +751,14 @@ class SparkSqlAstBuilder extends AstBuilder {
(Nil, Option(name), props, recordHandler)
}
- val (inFormat, inSerdeClass, inSerdeProps, reader) =
+ val (outFormat, outSerdeClass, outSerdeProps, reader) =
format(
- inRowFormat, "hive.script.recordreader",
+ outRowFormat, "hive.script.recordreader",
"org.apache.hadoop.hive.ql.exec.TextRecordReader")
- val (outFormat, outSerdeClass, outSerdeProps, writer) =
+ val (inFormat, inSerdeClass, inSerdeProps, writer) =
format(
- outRowFormat, "hive.script.recordwriter",
+ inRowFormat, "hive.script.recordwriter",
Review Comment:
<img width="714" alt="截屏2023-01-03 上午10 55 45" src="https://user-images.githubusercontent.com/46485123/210294713-9d62a8bd-48ac-4564-aadb-90bc745d8f2f.png">
Seems reader/writer is not consistence with `in/outFormat`. The reader should be with outFormat.
I prefer `inFormat` keep behind `outFormat`
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/BaseScriptTransformationSuite.scala:
##########
@@ -670,6 +670,26 @@ abstract class BaseScriptTransformationSuite extends SparkPlanTest with SQLTestU
),
df.select($"col").collect())
}
+
+ test("SPARK-41790: Set TRANSFORM reader and writer's format correctly") {
+ withTempView("v") {
+ val df = Seq(
+ (1, 2)
+ ).toDF("a", "b")
+ df.createTempView("v")
+
+ checkAnswer(
+ sql(
+ s"""
+ |SELECT TRANSFORM(a, b)
+ | ROW FORMAT DELIMITED
+ | FIELDS TERMINATED BY ','
+ | USING 'cat'
+ | AS (c)
+ |FROM v
+ """.stripMargin), identity, Row("1,2") :: Nil)
Review Comment:
Reader, writer seems only used in HiveScriptTransformationExec, this UT should move to `HiveScriptTransformationSuite`
--
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] HyukjinKwon commented on pull request #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #39315:
URL: https://github.com/apache/spark/pull/39315#issuecomment-1371936958
Merged to master.
--
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] mattshma commented on a diff in pull request #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
Posted by GitBox <gi...@apache.org>.
mattshma commented on code in PR #39315:
URL: https://github.com/apache/spark/pull/39315#discussion_r1060524787
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/BaseScriptTransformationSuite.scala:
##########
@@ -670,6 +670,26 @@ abstract class BaseScriptTransformationSuite extends SparkPlanTest with SQLTestU
),
df.select($"col").collect())
}
+
+ test("SPARK-41790: Set TRANSFORM reader and writer's format correctly") {
+ withTempView("v") {
+ val df = Seq(
+ (1, 2)
+ ).toDF("a", "b")
+ df.createTempView("v")
+
+ checkAnswer(
+ sql(
+ s"""
+ |SELECT TRANSFORM(a, b)
+ | ROW FORMAT DELIMITED
+ | FIELDS TERMINATED BY ','
+ | USING 'cat'
+ | AS (c)
+ |FROM v
+ """.stripMargin), identity, Row("1,2") :: Nil)
Review Comment:
ok
--
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] mattshma commented on a diff in pull request #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
Posted by GitBox <gi...@apache.org>.
mattshma commented on code in PR #39315:
URL: https://github.com/apache/spark/pull/39315#discussion_r1060446152
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/BaseScriptTransformationSuite.scala:
##########
@@ -670,6 +670,26 @@ abstract class BaseScriptTransformationSuite extends SparkPlanTest with SQLTestU
),
df.select($"col").collect())
}
+
+ test("SPARK-41790: Set TRANSFORM reader and writer's format correctly") {
+ withTempView("v") {
+ val df = Seq(
+ (1, 2)
+ ).toDF("a", "b")
+ df.createTempView("v")
+
+ checkAnswer(
+ sql(
+ s"""
+ |SELECT TRANSFORM(a, b)
+ | ROW FORMAT DELIMITED
+ | FIELDS TERMINATED BY ','
+ | USING 'cat'
+ | AS (c)
+ |FROM v
+ """.stripMargin), identity, Row("1,2") :: Nil)
Review Comment:
This UT will get nothing in currently master branch code, as [SPARK-41790](https://issues.apache.org/jira/projects/SPARK/issues/SPARK-41790) description said.
--
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] AmplabJenkins commented on pull request #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #39315:
URL: https://github.com/apache/spark/pull/39315#issuecomment-1367987064
Can one of the admins verify this patch?
--
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] HyukjinKwon commented on pull request #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #39315:
URL: https://github.com/apache/spark/pull/39315#issuecomment-1368137794
cc @AngersZhuuuu
--
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] AngersZhuuuu commented on a diff in pull request #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #39315:
URL: https://github.com/apache/spark/pull/39315#discussion_r1060468436
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/BaseScriptTransformationSuite.scala:
##########
@@ -670,6 +670,26 @@ abstract class BaseScriptTransformationSuite extends SparkPlanTest with SQLTestU
),
df.select($"col").collect())
}
+
+ test("SPARK-41790: Set TRANSFORM reader and writer's format correctly") {
+ withTempView("v") {
+ val df = Seq(
+ (1, 2)
+ ).toDF("a", "b")
+ df.createTempView("v")
+
+ checkAnswer(
+ sql(
+ s"""
+ |SELECT TRANSFORM(a, b)
+ | ROW FORMAT DELIMITED
+ | FIELDS TERMINATED BY ','
+ | USING 'cat'
+ | AS (c)
+ |FROM v
+ """.stripMargin), identity, Row("1,2") :: Nil)
Review Comment:
> This UT will get nothing in currently master branch code, as [SPARK-41790](https://issues.apache.org/jira/projects/SPARK/issues/SPARK-41790) description said.
Can you add desc in jira into this pr's desc?
--
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] mattshma commented on a diff in pull request #39315: [SPARK-41790][SQL] Set TRANSFORM reader and writer's format correctly
Posted by GitBox <gi...@apache.org>.
mattshma commented on code in PR #39315:
URL: https://github.com/apache/spark/pull/39315#discussion_r1060433518
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala:
##########
@@ -751,15 +751,15 @@ class SparkSqlAstBuilder extends AstBuilder {
(Nil, Option(name), props, recordHandler)
}
- val (inFormat, inSerdeClass, inSerdeProps, reader) =
+ val (inFormat, inSerdeClass, inSerdeProps, writer) =
Review Comment:
ok
--
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