Skip to content

[SPARK-32105][SQL]Refactor current ScriptTransformationExec code - #27983

Closed
AngersZhuuuu wants to merge 26 commits into
apache:masterfrom
AngersZhuuuu:follow_spark_15694
Closed

[SPARK-32105][SQL]Refactor current ScriptTransformationExec code#27983
AngersZhuuuu wants to merge 26 commits into
apache:masterfrom
AngersZhuuuu:follow_spark_15694

Conversation

@AngersZhuuuu

@AngersZhuuuuAngersZhuuuu commented Mar 23, 2020

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

  • Renamed hive transform scrip class hive/execution/ScriptTransformationExec to hive/execution/HiveScriptTransformationExec (don't rename file)
  • Extract class BaseScriptTransformationExec about common code used across SparkScriptTransformationExec(next pr add this) and HiveScriptTransformationExec
  • Extract class BaseScriptTransformationWriterThread of writing data thread across SparkScriptTransformationWriterThread(added next for support transform in sql/core ) and HiveScriptTransformationWriterThread ,
  • HiveScriptTransformationWriterThread additionally supports Hive serde format
  • Rename current Script strategies in hive module to HiveScript, in next pr will add SparkScript strategies for support transform in sql/core.

Todo List;

Why are the changes needed?

Support run transform in SQL mode without hive

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

Added UT

(3, "3", 3.0, BigDecimal(3.0), new Timestamp(3), Date.valueOf("2015-05-23"))
).toDF("a", "b", "c", "d", "e", "f") // Note column d's data type is Decimal(38, 18)
df.createTempView("v")

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here add test case of Timestamp and DateType

@AngersZhuuuuAngersZhuuuu changed the title [SPARK-15694][SQL][FOLLOW-UP] Implement ScriptTransformation in sql/core (part 1) [SPARK-15694][SQL][FOLLOW-UP] Implement ScriptTransformation in sql/coreMar 23, 2020
@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

@gatorsmile@tejasapatil@wangyum

I have restart implement ScriptTransform in sql/core now,

Rewrite origin pr in current code. Hope for your review and suggestion

@dongjoon-hyun

Copy link
Copy Markdown
Member

Hi, @AngersZhuuuu . This is too big as a follow-up. Please create a new JIRA issue for this.

@dongjoon-hyun

Copy link
Copy Markdown
Member

ok to test

@AngersZhuuuuAngersZhuuuu changed the title [SPARK-15694][SQL][FOLLOW-UP] Implement ScriptTransformation in sql/core[SPARK-15694][SQL]Implement ScriptTransformation in sql/coreApr 2, 2020
@SparkQA

Copy link
Copy Markdown

Test build #120696 has finished for PR 27983 at commit 0d9c437.

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

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

Hi, @AngersZhuuuu . This is too big as a follow-up. Please create a new JIRA issue for this.

Seems not need to create new jira? just delete follow-up?

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

@dongjoon-hyun
In latest test,

org.apache.spark.sql.execution.script.ScriptTransformationSuite.SPARK-14400 script transformation should fail for bad script command

Seem sometimes it won't catch the subprocess's exception.
I tried a lot of times, sometimes failed, sometimes success. May have problem about catch subproc's exception.

@SparkQA

Copy link
Copy Markdown

Test build #122194 has finished for PR 27983 at commit 6fcb1af.

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

@SparkQA

Copy link
Copy Markdown

Test build #122214 has finished for PR 27983 at commit 1e7c3df.

  • This patch fails from timeout after a configured wait of 400m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

cc @cloud-fan Can you take a look?

@SparkQA

Copy link
Copy Markdown

Test build #122637 has finished for PR 27983 at commit aea128b.

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

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

Test build #122637 has finished for PR 27983 at commit aea128b.

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

@dongjoon-hyun For the latest failed test, Did I miss something?

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

Retest this please

@SparkQA

Copy link
Copy Markdown

Test build #123517 has finished for PR 27983 at commit aea128b.

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

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #123525 has finished for PR 27983 at commit aea128b.

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

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #125324 has finished for PR 27983 at commit f52f376.

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

import org.apache.spark.sql.types.DataType
import org.apache.spark.util.{CircularBuffer, SerializableConfiguration, Utils}

trait BaseScriptTransform extends UnaryExecNode {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad. Probably, BaseScriptTransformationExec is better.. #27983 (comment)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad. Probably, BaseScriptTransformationExec is better.. #27983 (comment)

Yea, done

@maropu

Copy link
Copy Markdown
Member

Could you update the PR description, too?

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

Could you update the PR description, too?

Done, sorry for always forgot to do that....

@SparkQA

Copy link
Copy Markdown

Test build #125424 has finished for PR 27983 at commit 28cb9ae.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu

Copy link
Copy Markdown
Member

Could you check the comment, too? #27983 (comment)

@maropu

Copy link
Copy Markdown
Member

Could you update the PR description, too?

Done, sorry for always forgot to do that....

Renamed hive transform scrip class hive/execution/ScriptTransformationExec to hive/execution/script/HiveScriptTransformationExec (don't rename file)

/script/?

@SparkQA

Copy link
Copy Markdown

Test build #125431 has finished for PR 27983 at commit fcb0957.

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

@SparkQA

Copy link
Copy Markdown

Test build #125428 has finished for PR 27983 at commit e5cd3d8.

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

@cloud-fan

Copy link
Copy Markdown
Contributor

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #125452 has finished for PR 27983 at commit fcb0957.

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

@maropu

Copy link
Copy Markdown
Member

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #125461 has finished for PR 27983 at commit fcb0957.

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

@cloud-fan

Copy link
Copy Markdown
Contributor

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #125492 has started for PR 27983 at commit fcb0957.

@maropu

Copy link
Copy Markdown
Member

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #125537 has finished for PR 27983 at commit fcb0957.

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

@cloud-fan

Copy link
Copy Markdown
Contributor

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #125567 has finished for PR 27983 at commit fcb0957.

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

@maropu

Copy link
Copy Markdown
Member

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #125587 has started for PR 27983 at commit fcb0957.

@maropu

Copy link
Copy Markdown
Member

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #125645 has finished for PR 27983 at commit fcb0957.

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

@cloud-fan

Copy link
Copy Markdown
Contributor

thanks, merging to master!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@AngersZhuuuu@dongjoon-hyun@SparkQA@cloud-fan@HyukjinKwon@maropu@viirya