[SPARK-25374][SQL] SafeProjection supports fallback to an interpreted mode #22468

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.

Sign up for GitHub

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

Jump to bottom

Closed

maropu wants to merge 6 commits into apache:master from maropu:SPARK-25374-3

Member

maropu commented Sep 19, 2018

What changes were proposed in this pull request?

In SPARK-23711, we have implemented the expression fallback logic to an interpreted mode. So, this pr fixed code to support the same fallback mode in SafeProjection based on CodeGeneratorWithInterpretedFallback.

How was this patch tested?

Add tests in CodeGeneratorWithInterpretedFallbackSuite and UnsafeRowConverterSuite.

SparkQA commented Sep 19, 2018

Test build #96251 has finished for PR 22468 at commit 96b0f62.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class InterpretedSafeProjection(expressions: Seq[Expression]) extends Projection

maropu force-pushed the SPARK-25374-3 branch from 96b0f62 to d8f55da Compare

September 19, 2018 13:00

SparkQA commented Sep 19, 2018

Test build #96252 has finished for PR 22468 at commit d8f55da.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class InterpretedSafeProjection(expressions: Seq[Expression]) extends Projection

maropu force-pushed the SPARK-25374-3 branch from d8f55da to 0ee6a00 Compare

September 19, 2018 14:35

SparkQA commented Sep 19, 2018

Test build #96261 has finished for PR 22468 at commit 0ee6a00.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class InterpretedSafeProjection(expressions: Seq[Expression]) extends Projection

maropu force-pushed the SPARK-25374-3 branch from 0ee6a00 to bc5f144 Compare

September 19, 2018 14:42

SparkQA commented Sep 19, 2018

Test build #96262 has finished for PR 22468 at commit bc5f144.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class InterpretedSafeProjection(expressions: Seq[Expression]) extends Projection

Member Author

maropu commented Oct 4, 2018

retest this please

SparkQA commented Oct 4, 2018

Test build #96918 has finished for PR 22468 at commit bc5f144.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class InterpretedSafeProjection(expressions: Seq[Expression]) extends Projection

maropu force-pushed the SPARK-25374-3 branch from bc5f144 to f658ac9 Compare

October 15, 2018 12:55

SparkQA commented Oct 15, 2018

Test build #97393 has finished for PR 22468 at commit f658ac9.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class InterpretedSafeProjection(expressions: Seq[Expression]) extends Projection

Member Author

maropu commented Oct 16, 2018

cc: @cloud-fan @viirya

cloud-fan reviewed

View reviewed changes

...yst/src/main/scala/org/apache/spark/sql/catalyst/expressions/InterpretedSafeProjection.scala Outdated

Contributor

cloud-fan Oct 16, 2018

does SafeProjection need to handle NoOp? It's only used with MutableProjection in aggregate.

Member Author

maropu Oct 23, 2018

IIUC the input expressions in UnsafeProjection possibly have NoOps passed from aggregate expressions? So, IIUC GenerateSafeProjection handles NoOps here:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateSafeProjection.scala

Line 153 in 3b45567

case (NoOp, _) => ""

I'm not 100% sure though...

cloud-fan reviewed

View reviewed changes

...yst/src/main/scala/org/apache/spark/sql/catalyst/expressions/InterpretedSafeProjection.scala Outdated

Contributor

cloud-fan Oct 16, 2018

CodeGenerator.isPrimitiveType

Member Author

maropu Oct 23, 2018 •

edited

Loading

ok. ~~BTW, isPrimitive is a general helper function, so can we move this func. from CodeGenerator to an other place, e.g., object DataType?~~ NVM, we don't need isPrimitivieType here.

cloud-fan reviewed

View reviewed changes

...yst/src/main/scala/org/apache/spark/sql/catalyst/expressions/InterpretedSafeProjection.scala Outdated

Contributor

cloud-fan Oct 16, 2018

Let's not add this optimization at the beginning. We can add it later with a benchmark.

Member Author

maropu Oct 23, 2018

ok

cloud-fan reviewed

View reviewed changes

...yst/src/main/scala/org/apache/spark/sql/catalyst/expressions/InterpretedSafeProjection.scala Outdated

Contributor

cloud-fan Oct 16, 2018

ditto

cloud-fan reviewed

View reviewed changes

...yst/src/main/scala/org/apache/spark/sql/catalyst/expressions/InterpretedSafeProjection.scala Outdated

Contributor

cloud-fan Oct 16, 2018

shall we share it with other interpreted projections?

Member Author

maropu Oct 23, 2018 •

edited

Loading

~~ok, I'll brush up.~~ The master doesn't have this logic yet in the other interpreted projections, and #22512 has the same logic. So, I'll fix #22512 first, then share it in this pr.

cloud-fan reviewed

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/UserDefinedTypeSuite.scala Outdated

Contributor

cloud-fan Oct 16, 2018

why do we change it?

Member Author

maropu Oct 23, 2018

That's because we use MyDenseVectorUDT in UnsafeRowConverterSuite.scala for unit tests. (MyDenseVectorUDT is located in the core, but UnsafeRowConverterSuite located in the catalyst).

maropu force-pushed the SPARK-25374-3 branch from f658ac9 to 9d7b519 Compare

October 23, 2018 02:13

SparkQA commented Oct 23, 2018

Test build #97891 has finished for PR 22468 at commit 9d7b519.

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

SparkQA commented Oct 24, 2018

Test build #97963 has finished for PR 22468 at commit 2b25d09.

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

maropu force-pushed the SPARK-25374-3 branch from 2b25d09 to 1c5231e Compare

December 3, 2018 05:32

SparkQA commented Dec 3, 2018

Test build #99592 has finished for PR 22468 at commit 1c5231e.

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

Member Author

maropu commented Dec 3, 2018

retest this please

SparkQA commented Dec 3, 2018

Test build #99610 has finished for PR 22468 at commit 1c5231e.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan reviewed

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/InternalRow.scala Outdated

Contributor

cloud-fan Dec 3, 2018

We can rebase now

Member Author

maropu Dec 4, 2018

ok

maropu added 3 commits

December 4, 2018 09:29


          Implement InterpretedSafeProjection

c549d3e

Fix

127fd27

Fix

cec8480

maropu force-pushed the SPARK-25374-3 branch from 1c5231e to cec8480 Compare

December 4, 2018 00:30

SparkQA commented Dec 4, 2018

Test build #99639 has finished for PR 22468 at commit cec8480.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Fix

0b23adb

cloud-fan reviewed

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala

    
              }

              /**

               * A projection that could turn UnsafeRow into GenericInternalRow

Contributor

cloud-fan Dec 4, 2018

can we keep this comment?

cloud-fan reviewed

View reviewed changes

...la/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala

    
                  val exprs = Seq(Add(BoundReference(0, IntegerType, nullable = true), Literal.create(1)), NoOp)

                  val input = InternalRow.fromSeq(1 :: 1 :: Nil)

                  val expected = 2 :: null :: Nil

                  withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenOnly) {

Contributor

cloud-fan Dec 4, 2018

can we use testWithBothCodegenAndIntepreted?

Contributor

cloud-fan Dec 4, 2018

nvm, this is the code style in this test suite

SparkQA commented Dec 4, 2018

Test build #99641 has finished for PR 22468 at commit 0b23adb.

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

cloud-fan reviewed

View reviewed changes

...alyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/UnsafeRowConverterSuite.scala Outdated

    
                  // Since `ArrayBasedMapData` does not override `equals` and `hashCode`,

                  // we need to take care of it to compare rows.

                  def toComparable(d: Any): Any = d match {

Contributor

cloud-fan Dec 4, 2018

this does nothing, isn't it?

Member Author

maropu Dec 4, 2018

Since we cannot compare ArrayBasedMapDatas directly (that is, assert(mapResultRow === mapExpectedRow) fails), I just converted them into the Seqs of keys/values by this method.

cloud-fan approved these changes

View reviewed changes

Fix

7ef5f86

cloud-fan reviewed

View reviewed changes

...alyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/UnsafeRowConverterSuite.scala Outdated

    
                  val mapResultRow = convertBackToInternalRow(mapRow, fields4).toSeq(fields4)

                  val mapExpectedRow = mapRow.toSeq(fields4)

                  // Since `ArrayBasedMapData` does not override `equals` and `hashCode`,

Contributor

cloud-fan Dec 4, 2018

maybe we should implement equals and hashCode in ArrayBasedMapData and UnsafeMapData.

Contributor

cloud-fan Dec 4, 2018

Or we can use ExpressionEvalHelper.checkResult here.

Member Author

maropu Dec 4, 2018

fixed code to use ExpressionEvalHelper.checkResult.

I don't remember correctly though, we might have some historical reasons about that; ArrayBasedMapData has no hashCode and equals. Probably, somebody might know this... cc: @hvanhovell @viirya

Contributor

hvanhovell Dec 4, 2018

ArrayBasedMapData/UnsafeMapData does not have equals() or hashCode() implemented because we do not have a good story around map equality. Implementing equals/hashcode for map is only half of the solution, we would also need a comparable binary format.

Member Author

maropu Dec 5, 2018

Aha, thanks. I remember that its related to SPARK-18134.

Fix

fbfbbff

SparkQA commented Dec 4, 2018

Test build #99650 has finished for PR 22468 at commit fbfbbff.

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

SparkQA commented Dec 4, 2018

Test build #99647 has finished for PR 22468 at commit 7ef5f86.

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

Member Author

maropu commented Dec 4, 2018

retest this please

SparkQA commented Dec 4, 2018

Test build #99655 has finished for PR 22468 at commit fbfbbff.

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

Contributor

cloud-fan commented Dec 4, 2018

thanks, merging to master!

asfgit closed this in

jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request


          [SPARK-25374][SQL] SafeProjection supports fallback to an interpreted…

f83ea93

… mode

## What changes were proposed in this pull request?
In SPARK-23711, we have implemented the expression fallback logic to an interpreted mode. So, this pr fixed code to support the same fallback mode in `SafeProjection` based on `CodeGeneratorWithInterpretedFallback`.

## How was this patch tested?
Add tests in `CodeGeneratorWithInterpretedFallbackSuite` and `UnsafeRowConverterSuite`.

Closes apache#22468 from maropu/SPARK-25374-3.

Authored-by: Takeshi Yamamuro <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>

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

Labels

None yet