Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 29.4k
[SPARK-32388][SQL] TRANSFORM with schema-less mode should keep the same with hive#29421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8ac3de399ae89154f7993f8548746b4071ca906818bd940711e78b445f03222191bcceedec9bb02653a0f745060e83f6a5File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -111,15 +111,14 @@ trait BaseScriptTransformationExec extends UnaryExecNode { | ||
| .zip(outputFieldWriters) | ||
| .map { case (data, writer) => writer(data) }) | ||
| } else { | ||
| // In schema less mode, hive default serde will choose first two output column as output | ||
| // if output column size less then 2, it will throw ArrayIndexOutOfBoundsException. | ||
| // Here we change spark's behavior same as hive's default serde. | ||
| // But in hive, TRANSFORM with schema less behavior like origin spark, we will fix this | ||
| // to keep spark and hive behavior same in SPARK-32388 | ||
| // In schema less mode, hive will choose first two output column as output. | ||
| // If output column size less then 2, it will return NULL for columns with missing values. | ||
| // Here we split row string and choose first 2 values, if values's size less then 2, | ||
| // we pad NULL value until 2 to make behavior same with hive. | ||
| val kvWriter = CatalystTypeConverters.createToCatalystConverter(StringType) | ||
| prevLine: String => | ||
| new GenericInternalRow( | ||
| prevLine.split(outputRowFormat).slice(0, 2) | ||
| prevLine.split(outputRowFormat).slice(0, 2).padTo(2, null) | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AngersZhuuuu, can you add a configuration to control the legacy behaviour, and add a note to the migration guide since it's arguable to say this is a bug? With that, I think I can leave my sign-off and merge. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This suggestion looks good to me, but since we have fix a similar issue. c75a827#diff-1bdf8d74f6b4f84d5acb1d1490503cde337fe64fe5d21339d7fedb06bd7cabc0 How about merge this and I will follow a pr to add a configuration to control the legacy behavior. Change these two point together seems to be better? Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGMT | ||
| .map(kvWriter)) | ||
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maropu and will rase a new pr for https://issues.apache.org/jira/browse/SPARK-32667 ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maropu How about split this fix as a new one, since this fix is very necessary and no disputed Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's a trivial fix, IMO its okay to include it in this PR. | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -785,7 +785,9 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) { | ||
| // Use default (serde) format. | ||
| val name = conf.getConfString("hive.script.serde", | ||
| "org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe") | ||
| val props = Seq("field.delim" -> "\t") | ||
| val props = Seq( | ||
| "field.delim" -> "\t", | ||
| "serialization.last.column.takes.rest" -> "true") | ||
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cloud-fan@maropu will take rest columns for last column as default. so changed here. Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't respect ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And if user specify Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. | ||
| val recordHandler = Option(conf.getConfString(configKey, defaultConfigValue)) | ||
| (Nil, Option(name), props, recordHandler) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -156,10 +156,7 @@ class HiveScriptTransformationSuite extends BaseScriptTransformationSuite with T | ||
| assert(uncaughtExceptionHandler.exception.isEmpty) | ||
| } | ||
| test("SPARK-25990: TRANSFORM should handle schema less correctly (hive serde)") { | ||
| assume(TestUtils.testCommandAvailable("python")) | ||
| val scriptFilePath = copyAndGetResourceFile("test_script.py", ".py").getAbsolutePath | ||
| test("SPARK-32388: TRANSFORM should handle schema less correctly (hive serde)") { | ||
| withTempView("v") { | ||
| val df = Seq( | ||
| (1, "1", 1.0, BigDecimal(1.0), new Timestamp(1)), | ||
| @@ -168,21 +165,157 @@ class HiveScriptTransformationSuite extends BaseScriptTransformationSuite with T | ||
| ).toDF("a", "b", "c", "d", "e") // Note column d's data type is Decimal(38, 18) | ||
| df.createTempView("v") | ||
| val query = sql( | ||
| s""" | ||
| |SELECT TRANSFORM(a, b, c, d, e) | ||
| |USING 'python ${scriptFilePath}' | ||
| |FROM v | ||
| """.stripMargin) | ||
| // In hive default serde mode, if we don't define output schema, | ||
| // when output column size > 2 and don't specify serde, | ||
| // it will choose take rest columns in second column as output schema | ||
| // (key: String, value: String) | ||
| checkAnswer( | ||
| sql( | ||
| s""" | ||
| |SELECT TRANSFORM(a, b, c, d, e) | ||
| | USING 'cat' | ||
| |FROM v | ||
| """.stripMargin), | ||
| identity, | ||
| df.select( | ||
| 'a.cast("string").as("key"), | ||
| concat_ws("\t", | ||
| 'b.cast("string"), | ||
| 'c.cast("string"), | ||
| 'd.cast("string"), | ||
| 'e.cast("string")).as("value")).collect()) | ||
| // In hive default serde mode, if we don't define output schema, | ||
| // when output column size > 2 and just specify serde, | ||
| // it will choose take rest columns in second column as output schema | ||
| // (key: String, value: String) | ||
| checkAnswer( | ||
| sql( | ||
| s""" | ||
| |SELECT TRANSFORM(a, b, c, d, e) | ||
| | ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe' | ||
| | WITH SERDEPROPERTIES ( | ||
| | 'field.delim' = '\t' | ||
| | ) | ||
| | USING 'cat' | ||
| | ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe' | ||
| | WITH SERDEPROPERTIES ( | ||
| | 'field.delim' = '\t' | ||
| | ) | ||
| |FROM v | ||
| """.stripMargin), | ||
| identity, | ||
| df.select( | ||
| 'a.cast("string").as("key"), | ||
| 'b.cast("string").as("value")).collect()) | ||
| // In hive default serde mode, if we don't define output schema, | ||
| // when output column size > 2 and specify serde with | ||
| // 'serialization.last.column.takes.rest=true', | ||
| // it will choose take rest columns in second column as output schema | ||
| // (key: String, value: String) | ||
| checkAnswer( | ||
| sql( | ||
| s""" | ||
| |SELECT TRANSFORM(a, b, c, d, e) | ||
| | ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe' | ||
| | WITH SERDEPROPERTIES ( | ||
| | 'field.delim' = '\t', | ||
| | 'serialization.last.column.takes.rest' = 'true' | ||
| | ) | ||
| | USING 'cat' | ||
| | ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe' | ||
| | WITH SERDEPROPERTIES ( | ||
| | 'field.delim' = '\t', | ||
| | 'serialization.last.column.takes.rest' = 'true' | ||
| | ) | ||
| |FROM v | ||
| """.stripMargin), | ||
| identity, | ||
| df.select( | ||
| 'a.cast("string").as("key"), | ||
| concat_ws("\t", | ||
| 'b.cast("string"), | ||
| 'c.cast("string"), | ||
| 'd.cast("string"), | ||
| 'e.cast("string")).as("value")).collect()) | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if a single column given with hive serde in this case? https://github.com/apache/spark/pull/29414/files#diff-01228f8ade90c259db2dfdf31ea8a5d1R61-R63 ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
See #29421 (comment) Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see. In the case, we should return NULL. | ||
| // In hive default serde mode, if we don't define output schema, | ||
| // when output column size > 2 and specify serde | ||
| // with 'serialization.last.column.takes.rest=false', | ||
| // it will choose first two column as output schema (key: String, value: String) | ||
| checkAnswer( | ||
| sql( | ||
| s""" | ||
| |SELECT TRANSFORM(a, b, c, d, e) | ||
| | ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe' | ||
| | WITH SERDEPROPERTIES ( | ||
| | 'field.delim' = '\t', | ||
| | 'serialization.last.column.takes.rest' = 'false' | ||
| | ) | ||
| | USING 'cat' | ||
| | ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe' | ||
| | WITH SERDEPROPERTIES ( | ||
| | 'field.delim' = '\t', | ||
| | 'serialization.last.column.takes.rest' = 'false' | ||
| | ) | ||
| |FROM v | ||
| """.stripMargin), | ||
| identity, | ||
| df.select( | ||
| 'a.cast("string").as("key"), | ||
| 'b.cast("string").as("value")).collect()) | ||
| // In hive default serde mode, if we don't define output schema, it will choose first | ||
| // two column as output schema (key: String, value: String) | ||
| // In hive default serde mode, if we don't define output schema, | ||
| // when output column size = 2 and specify serde, it will these two column as | ||
| // output schema (key: String, value: String) | ||
| checkAnswer( | ||
| query, | ||
| sql( | ||
| s""" | ||
| |SELECT TRANSFORM(a, b) | ||
| | ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe' | ||
| | WITH SERDEPROPERTIES ( | ||
| | 'field.delim' = '\t', | ||
| | 'serialization.last.column.takes.rest' = 'true' | ||
| | ) | ||
| | USING 'cat' | ||
| | ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe' | ||
| | WITH SERDEPROPERTIES ( | ||
| | 'field.delim' = '\t', | ||
| | 'serialization.last.column.takes.rest' = 'true' | ||
| | ) | ||
| |FROM v | ||
| """.stripMargin), | ||
| identity, | ||
| df.select( | ||
| 'a.cast("string").as("key"), | ||
| 'b.cast("string").as("value")).collect()) | ||
| // In hive default serde mode, if we don't define output schema, | ||
| // when output column size < 2 and specify serde, it will return null for deficiency | ||
| // output schema (key: String, value: String) | ||
| checkAnswer( | ||
| sql( | ||
| s""" | ||
| |SELECT TRANSFORM(a) | ||
| | ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe' | ||
| | WITH SERDEPROPERTIES ( | ||
| | 'field.delim' = '\t', | ||
| | 'serialization.last.column.takes.rest' = 'true' | ||
| | ) | ||
| | USING 'cat' | ||
| | ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe' | ||
| | WITH SERDEPROPERTIES ( | ||
| | 'field.delim' = '\t', | ||
| | 'serialization.last.column.takes.rest' = 'true' | ||
| | ) | ||
| |FROM v | ||
| """.stripMargin), | ||
| identity, | ||
| df.select( | ||
| 'a.cast("string").as("key"), | ||
| lit(null)).collect()) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could you leave comments about why we need
.padTo(2, null)here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the latest change, I have changed comment leaved before