Skip to content
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

[SPARK-48296][SQL] Codegen Support for to_xml #46591

Closed
wants to merge 6 commits into from

Conversation

panbingkun
Copy link
Contributor

@panbingkun panbingkun commented May 15, 2024

What changes were proposed in this pull request?

The PR adds Codegen Support for to_xml.

Why are the changes needed?

Improve codegen coverage.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • Add new UT & existed UT.
  • Pass GA.

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the SQL label May 15, 2024
@@ -55,7 +55,7 @@ class XmlFunctionsSuite extends QueryTest with SharedSparkSession {
val options = Map("rowTag" -> "foo").asJava

checkAnswer(
df.select(from_xml($"value", schema)),
df.select(from_xml($"value", schema, options)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Obviously, its usage does not match the title of UT

@panbingkun panbingkun changed the title [MINOR] Fix typo for XmlFunctionsSuite [SPARK-48296][SQL] Codegen Support for to_xml May 16, 2024
@@ -186,9 +186,6 @@ case class SchemaOfXml(
@transient
private lazy val xmlOptions = new XmlOptions(options, "UTC")

@transient
private lazy val xmlFactory = xmlOptions.buildXmlFactory()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variable xmlFactory is not used in the expression SchemaOfXml, so it is deleted here. It has no strong relationship with this PR.

@panbingkun
Copy link
Contributor Author

cc @yaooqinn @cloud-fan

@panbingkun panbingkun marked this pull request as ready for review May 16, 2024 03:32

override def inputTypes: Seq[AbstractDataType] = StructType :: Nil

override def prettyName: String = "to_xml"

override protected def withNewChildInternal(newChild: Expression): StructsToXml =
copy(child = newChild)

override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
Copy link
Member

Choose a reason for hiding this comment

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

Simply

 override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
    val expr = ctx.addReferenceObj("this", this)
    defineCodeGen(ctx, ev, input => s"(UTF8String) $expr.nullSafeEval($input)")
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, updated.
thanks!

Copy link
Member

Choose a reason for hiding this comment

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

We can also revert other refactoring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me have a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

….scala

Co-authored-by: Kent Yao <yao@apache.org>
@yaooqinn yaooqinn closed this in fa83d0f May 16, 2024
@yaooqinn
Copy link
Member

Thanks @panbingkun, merged to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants