[SPARK-32403][SQL] Refactor current ScriptTransformationExec #29199

AngersZhuuuu · 2020-07-23T04:59:19Z

What changes were proposed in this pull request?

This PR comes from the comment: #29085 (comment)

Extract common Script IOSchema ScriptTransformationIOSchema
avoid repeated judgement extract process output row method createOutputIteratorWithoutSerde && createOutputIteratorWithSerde
add default no serde IO schemas ScriptTransformationIOSchema.defaultIOSchema

Why are the changes needed?

Refactor code

Does this PR introduce any user-facing change?

NO

How was this patch tested?

NO

… code

AngersZhuuuu · 2020-07-23T05:01:29Z

FYI @maropu

maropu · 2020-07-23T06:52:36Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala

      case NullType => voidTypeInfo
+      case dt =>
+        throw new AnalysisException(
+          s"${dt.getClass.getSimpleName.replace("$", "")} cannot be converted to Hive TypeInfo")


This is related to this reafactoring?

This is related to this reafactoring?

So make this change as a seperated jira?

I think this is a testing issue (2.): #29085 (comment)

I think this is a testing issue (2.): #29085 (comment)

OK, change in next pr(2)

maropu · 2020-07-23T06:53:58Z

FYI: This PR comes from the comment: #29085 (comment)

maropu · 2020-07-23T06:55:07Z

Could you file a new JIRA?

maropu · 2020-07-23T06:57:10Z

Anyone could check this? @cloud-fan @viirya @wangyum

SparkQA · 2020-07-23T07:05:01Z

Test build #126382 has finished for PR 29199 at commit 9b1f28a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-23T07:05:02Z

Test build #126381 has finished for PR 29199 at commit a007fa1.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-23T07:05:02Z

Test build #126378 has finished for PR 29199 at commit 8364f1f.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

AngersZhuuuu · 2020-07-23T09:12:16Z

Could you file a new JIRA?

Done

SparkQA · 2020-07-23T12:41:40Z

Test build #126393 has finished for PR 29199 at commit 01bf8fb.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AngersZhuuuu · 2020-07-27T01:57:33Z

Any update?

cloud-fan · 2020-07-30T10:49:58Z

add default no serde IO schemas ScriptTransformationIOSchema.defaultIOSchema

I think we will have a default native serde. So for now we just need a fake one which just uses cast to string?

AngersZhuuuu · 2020-07-31T01:35:16Z

add default no serde IO schemas ScriptTransformationIOSchema.defaultIOSchema

I think we will have a default native serde. So for now we just need a fake one which just uses cast to string?

@alfozan will add their spark serde , and will have a default one #29085 (comment)

AngersZhuuuu · 2020-07-31T01:45:23Z

add default no serde IO schemas ScriptTransformationIOSchema.defaultIOSchema

I think we will have a default native serde. So for now we just need a fake one which just uses cast to string?

For @maropu's advice. more detail is :

refactor more code about ScriptTransformationExec (this pr)
add more test case for current HiveScriptTransformationExec (since we have too less UT for transform and there are many small point need to be clear)
Implement SparkScriptTransformationExec in sql/core and add more detail UT
Alfozan will add spark serde used in there prod, and then will have a default spark serde
I will add some small PR to fix some small point , such as 1. support array/map/struct in noserde mode. 2. schema less data different form hive 's behavior etc....

AngersZhuuuu · 2020-08-03T01:50:00Z

It‘s ok to merge this and start to work on follow step ?

cloud-fan · 2020-08-03T06:20:31Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationExec.scala

+
+    // This nullability is a performance optimization in order to avoid an Option.foreach() call
+    // inside of a loop
+    @Nullable val (inputSerde, inputSoi) = initInputSerDe(ioschema, input).getOrElse((null, null))


why do we use @Nullable for local variables?

why do we use @Nullable for local variables?

Seems first author add this to remind the value could be null and add judgement at next code.
Seems a good coding style ? Sure it's ok to remove these, so just remove all @nullable?
cc @maropu

How about checking performance numbers with/without it?

How about checking performance numbers with/without it?

Seems not need, since we have remove these unnecessary judgement in each row's loop.
It's ok to just remove it.

cloud-fan · 2020-08-03T06:20:38Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationExec.scala

+
+    // This nullability is a performance optimization in order to avoid an Option.foreach() call
+    // inside of a loop
+    @Nullable val (outputSerde, outputSoi) = {


ditto

Removed

SparkQA · 2020-08-04T06:20:58Z

Test build #127016 has finished for PR 29199 at commit 927f260.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AngersZhuuuu · 2020-08-10T01:27:54Z

retest this please

SparkQA · 2020-08-10T03:15:26Z

Test build #127250 has finished for PR 29199 at commit 927f260.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AngersZhuuuu · 2020-08-10T03:16:42Z

retest this please

SparkQA · 2020-08-10T07:05:01Z

Test build #127259 has finished for PR 29199 at commit 927f260.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

AngersZhuuuu · 2020-08-10T07:05:45Z

retest this please

SparkQA · 2020-08-10T11:52:21Z

Test build #127272 has finished for PR 29199 at commit 927f260.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-08-10T16:37:17Z

thanks, merging to master!

AngersZhuuuu added 2 commits July 23, 2020 12:55

[SPARK-32105][SQL][FOLLOWUP]Refactor current ScriptTransformationExec…

8364f1f

… code

Update HiveScriptTransformationSuite.scala

f608fe5

probot-autolabeler bot added the SQL label Jul 23, 2020

AngersZhuuuu added 3 commits July 23, 2020 13:03

Update HiveScriptTransformationSuite.scala

83f3c2a

Update HiveInspectors.scala

a007fa1

Update BaseScriptTransformationExec.scala

9b1f28a

maropu reviewed Jul 23, 2020

View reviewed changes

maropu changed the title ~~[SPARK-32105][SQL][FOLLOWUP]Refactor current ScriptTransformationExec…~~ [SPARK-32105][SQL][FOLLOWUP] Refactor current ScriptTransformationExec Jul 23, 2020

AngersZhuuuu changed the title ~~[SPARK-32105][SQL][FOLLOWUP] Refactor current ScriptTransformationExec~~ [SPARK-32403][SQL] ScriptTransformExec extract common method from process row to avoid repeated judgement Jul 23, 2020

Update HiveInspectors.scala

01bf8fb

AngersZhuuuu changed the title ~~[SPARK-32403][SQL] ScriptTransformExec extract common method from process row to avoid repeated judgement~~ [SPARK-32403][SQL] Refactor current ScriptTransformationExec Jul 23, 2020

cloud-fan reviewed Aug 3, 2020

View reviewed changes

Update HiveScriptTransformationExec.scala

927f260

cloud-fan closed this in d251443 Aug 10, 2020

[SPARK-32403][SQL] Refactor current ScriptTransformationExec #29199

[SPARK-32403][SQL] Refactor current ScriptTransformationExec #29199

Uh oh!

Conversation

AngersZhuuuu commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

AngersZhuuuu commented Jul 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu commented Jul 23, 2020

Uh oh!

maropu commented Jul 23, 2020

Uh oh!

maropu commented Jul 23, 2020

Uh oh!

SparkQA commented Jul 23, 2020

Uh oh!

SparkQA commented Jul 23, 2020

Uh oh!

SparkQA commented Jul 23, 2020

Uh oh!

AngersZhuuuu commented Jul 23, 2020

Uh oh!

SparkQA commented Jul 23, 2020

Uh oh!

AngersZhuuuu commented Jul 27, 2020

Uh oh!

cloud-fan commented Jul 30, 2020

Uh oh!

AngersZhuuuu commented Jul 31, 2020

Uh oh!

AngersZhuuuu commented Jul 31, 2020

Uh oh!

AngersZhuuuu commented Aug 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 4, 2020

Uh oh!

AngersZhuuuu commented Aug 10, 2020

Uh oh!

SparkQA commented Aug 10, 2020

Uh oh!

AngersZhuuuu commented Aug 10, 2020

Uh oh!

SparkQA commented Aug 10, 2020

Uh oh!

AngersZhuuuu commented Aug 10, 2020

Uh oh!

SparkQA commented Aug 10, 2020

Uh oh!

cloud-fan commented Aug 10, 2020

Uh oh!

Reviewers

AngersZhuuuu commented Jul 23, 2020 •

edited

Loading

maropu Jul 23, 2020 •

edited

Loading

AngersZhuuuu Aug 3, 2020 •

edited

Loading