Skip to content

[SPARK-25426][SQL] Remove the duplicate fallback logic in UnsafeProjection#22417

Closed
maropu wants to merge 3 commits intoapache:masterfrom
maropu:SPARK-25426
Closed

[SPARK-25426][SQL] Remove the duplicate fallback logic in UnsafeProjection#22417
maropu wants to merge 3 commits intoapache:masterfrom
maropu:SPARK-25426

Conversation

@maropu
Copy link
Member

@maropu maropu commented Sep 14, 2018

What changes were proposed in this pull request?

This pr removed the duplicate fallback logic in UnsafeProjection.

This pr comes from #22355.

How was this patch tested?

Added tests in CodeGeneratorWithInterpretedFallbackSuite.

@SparkQA
Copy link

SparkQA commented Sep 14, 2018

Test build #96058 has finished for PR 22417 at commit 35e7911.

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

}

protected def createCodeGeneratedObject(in: IN): OUT
protected def createCodeGeneratedObject(in: IN, subexpressionEliminationEnabled: Boolean): OUT
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to get config directly from SQLConf, why do we need to pass it as a parameter here?

Can we keep this API untouched and let createCodeGeneratedObject implementation to take care of it?

Like:

object UnsafeProjection
    extends CodeGeneratorWithInterpretedFallback[Seq[Expression], UnsafeProjection] {

  override protected def createCodeGeneratedObject(in: Seq[Expression]): UnsafeProjection = {
    GenerateUnsafeProjection.generate(in, SQLConf.get.subexpressionEliminationEnabled)
  }

  ...
}

I think it is much simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, yes. Looks reasonable. I'll update soon.

This reverts commit 35e7911.
@SparkQA
Copy link

SparkQA commented Sep 14, 2018

Test build #96073 has finished for PR 22417 at commit ea7e473.

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

@maropu maropu changed the title [SPARK-25426][SQL] Handles subexpression elimination config inside CodeGeneratorWithInterpretedFallback [SPARK-25426][SQL] Remove the duplicate fallback logic in UnsafeProjection Sep 14, 2018
@SparkQA
Copy link

SparkQA commented Sep 14, 2018

Test build #96074 has finished for PR 22417 at commit e59902a.

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

@hvanhovell
Copy link
Contributor

LGTM

@viirya
Copy link
Member

viirya commented Sep 15, 2018

LGTM too.

@maropu
Copy link
Member Author

maropu commented Sep 15, 2018

cc: @cloud-fan

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks! Merged to master.

@asfgit asfgit closed this in 5ebef33 Sep 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants