[SPARK-23711][SQL] Add fallback generator for UnsafeProjection by viirya · Pull Request #21106 · apache/spark

viirya · 2018-04-19T14:15:29Z

What changes were proposed in this pull request?

Add fallback logic for UnsafeProjection. In production we can try to create unsafe projection using codegen implementation. Once any compile error happens, it fallbacks to interpreted implementation.

How was this patch tested?

Added test.

viirya · 2018-04-19T14:15:56Z

cc @hvanhovell

hvanhovell · 2018-04-19T15:47:54Z

+import org.codehaus.commons.compiler.CompileException
+import org.codehaus.janino.InternalCompilerException
+
+object CodegenObjectFactory {


Can we make this an abstract class in which we just need to implement hooks for the code generated and interpreted versions, i.e.:
objectCodegenError { defunapply(throwable: Throwable):Option[Exception] = throwable match { casee: InternalCompilerException=>Some(e) casee: CompileException=>Some(e) case _ =>None } } abstractclassCodegenObjectFactory[IN, OUT] { defcreate(in: IN):OUT=try createCodeGeneratedObject(in) catch { caseCodegenError(_) =>createInterpretedObject(in) } protecteddefcreateCodeGeneratedObject(in: IN):OUTprotecteddefcreateInterpretedObject(in: IN):OUT } objectUnsafeProjectionCreatorextendsCodegenObjectFactory[Seq[Expression], UnsafeProjection] { ... }

This looks good to me, except that I think UnsafeProjectionCreator should still support UnsafeProjectionCreator's APIs. So we can replace all usage of UnsafeProjection with UnsafeProjectionCreator.

SparkQA · 2018-04-19T17:48:15Z

Test build #89578 has finished for PR 21106 at commit fe2a1cd.

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

SparkQA · 2018-04-20T03:11:29Z

Test build #89603 has finished for PR 21106 at commit 36f90cf.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class CodegenObjectFactory[IN, OUT]

viirya · 2018-04-20T03:42:23Z

retest this please.

SparkQA · 2018-04-20T07:05:01Z

Test build #89604 has finished for PR 21106 at commit 36f90cf.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class CodegenObjectFactory[IN, OUT]

viirya · 2018-04-20T07:09:07Z

retest this please.

SparkQA · 2018-04-20T10:54:42Z

Test build #89615 has finished for PR 21106 at commit 36f90cf.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class CodegenObjectFactory[IN, OUT]

viirya · 2018-04-23T14:05:37Z

@hvanhovell Any more comments on the current design? If not, I will apply this to all places that we create unsafe projection.

hvanhovell · 2018-04-23T14:34:41Z

Yeah, let's move forward slowly. I am still pondering about the what the right abstraction here would look like; this looks promising though.

Can you try to unify this class with UnsafeProjectionCreator?

cloud-fan · 2018-04-23T15:18:09Z

I think it's very hard to unify the entry point(input type) for all the code generators. E.g. some use Seq[Expression] as input, some use Seq[DataType].

I'd like to make CodegenObjectFactory only define internal interfaces, and each implementation defines its own public interface by its need. E.g.

object UnsafeProjectionCreator
extends CodegenObjectFactory[(Seq[Expression], Boolean), UnsafeProjection] {
protected def createCodeGeneratedObject(
expressions: Seq[Expression],
subexpressionEliminationEnabled: Boolean) = ...
protected def createInterpretedObject(
expressions: Seq[Expression],
subexpressionEliminationEnabled: Boolean) = ...
def create(expressions: Seq[Expression]) = ...
def create(schema: StructType) = ...
}

viirya · 2018-04-25T14:56:29Z

I tried to unify this class with UnsafeProjectionCreator. So there is no more UnsafeProjectionCreator trait.

Without the trait UnsafeProjectionCreator, one problem is we can't easily test both against UnsafeProjection and InterpretedUnsafeProjection objects with the same interface of UnsafeProjectionCreator (e.g., UnsafeRowConverterSuite).

For test, I think we still need to test codegen and interpreted unsafe projection implementations individually as UnsafeRowConverterSuite does.

cloud-fan · 2018-04-26T11:26:29Z

I think UnsafeProjectionCreator is only useful for tests, because in production we should always try codegen first, and fallback to interpretation if codegen fails. There is no need to disable the fallback(always codegen) or skip the codegen(always interpretation) in production.

How about we provide a sql conf for test only, to be able to disable the fallback(always codegen) or skip the codegen(always interpretation)?

SparkQA · 2018-04-26T14:03:03Z

Test build #89882 has finished for PR 21106 at commit 68213fb.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait CodegenObjectFactoryBase[IN, OUT]
trait CodegenObjectFactory[IN, OUT] extends CodegenObjectFactoryBase[IN, OUT]
trait CodegenObjectFactoryWithoutFallback[IN, OUT] extends CodegenObjectFactoryBase[IN, OUT]
trait InterpretedCodegenObjectFactory[IN, OUT] extends CodegenObjectFactoryBase[IN, OUT]

viirya · 2018-04-27T12:29:15Z

A sql conf sounds good to me. @hvanhovell What do you think?

viirya · 2018-05-03T14:55:55Z

@hvanhovell If you don't have other comment, I will follow @cloud-fan's suggestion to add a sql conf for it. Thanks.

viirya · 2018-05-06T13:56:33Z

@hvanhovell @cloud-fan A SQL config was added to control codegen/interpreted for test. Please let me know if you have any comment on current approach. Thanks.

SparkQA · 2018-05-06T15:18:40Z

Test build #90277 has finished for PR 21106 at commit 32e2766.

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

SparkQA · 2018-05-06T16:09:08Z

Test build #90278 has finished for PR 21106 at commit aeef468.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class CodegenObjectFactorySuite extends SparkFunSuite with PlanTestBase

viirya · 2018-05-06T22:52:32Z

+ // Creates wanted object. First trying codegen implementation. If any compile error happens,
+ // fallbacks to interpreted version.
+ def createObject(in: IN): OUT = {
+ val fallbackMode = SQLConf.get.getConf(SQLConf.CODEGEN_OBJECT_FALLBACK)


We can only access SQLConf on the driver. Now this causes test failure. We can't know this config value inside createObject which can be called on executors.
To solve this, UnsafeProjection can't be an object as now, but a case class with a fallback mode parameter. So we can create this factory on driver. @cloud-fan @hvanhovell WDYT?

SparkQA · 2018-05-07T16:23:14Z

Test build #90313 has finished for PR 21106 at commit 06cc8cc.

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

SparkQA · 2018-05-07T17:21:49Z

Test build #90319 has finished for PR 21106 at commit 1f5cc17.

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

cloud-fan · 2018-05-17T10:40:59Z

- if (unsafeRow != expectedRow) {
- fail("Incorrect evaluation in unsafe mode: " +
- s"$expression, actual: $unsafeRow, expected: $expectedRow$input")
+ for (fallbackMode <- Seq("CODEGEN_ONLY", "NO_CODEGEN")) {


let's not hardcode it, call CodegenObjectFactoryMode.xxx instead

cloud-fan · 2018-05-17T10:41:50Z

- f(UnsafeProjection)
- f(InterpretedUnsafeProjection)
+ private def testBothCodegenAndInterpreted(name: String)(f: => Unit): Unit = {
+ for (fallbackMode <- Seq("CODEGEN_ONLY", "NO_CODEGEN")) {


SparkQA · 2018-05-17T11:59:38Z

Test build #90725 has finished for PR 21106 at commit 8c56ae4.

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

SparkQA · 2018-05-17T12:43:33Z

Test build #90724 has finished for PR 21106 at commit 716d88f.

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

SparkQA · 2018-05-18T07:05:02Z

Test build #90776 has finished for PR 21106 at commit 5a735af.

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

viirya · 2018-05-18T07:29:42Z

retest this please.

cloud-fan · 2018-05-18T08:03:05Z

also cc @rednaxelafx

rednaxelafx · 2018-05-18T11:29:40Z

Should this PR wait for #21299 so that accessing confs on executors can be made simpler?

SparkQA · 2018-05-18T11:52:50Z

Test build #90783 has finished for PR 21106 at commit 5a735af.

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

viirya · 2018-05-22T04:01:55Z

@cloud-fan @rednaxelafx Accessing confs are now simpler. Please review this again. Thanks.

SparkQA · 2018-05-22T06:44:30Z

Test build #90932 has finished for PR 21106 at commit ed525f6.

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

viirya · 2018-05-22T06:56:15Z

retest this please.

SparkQA · 2018-05-22T07:05:02Z

Test build #90939 has finished for PR 21106 at commit ed525f6.

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

kiszk · 2018-05-22T07:52:10Z

retest this please

kiszk · 2018-05-22T08:39:13Z

+ private def testBothCodegenAndInterpreted(name: String)(f: => Unit): Unit = {
+ val modes = Seq(CodegenObjectFactoryMode.CODEGEN_ONLY, CodegenObjectFactoryMode.NO_CODEGEN)
+ for (fallbackMode <- modes) {
+ test(name + " with " + fallbackMode) {


nit: s"$name with $fallbackMode"

SparkQA · 2018-05-22T09:34:20Z

Test build #90947 has finished for PR 21106 at commit ed525f6.

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

SparkQA · 2018-05-22T12:17:15Z

Test build #90951 has finished for PR 21106 at commit c9ec817.

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

cloud-fan · 2018-05-22T12:45:50Z

UnsafeKVExternalSorterSuite should not create a task context with null properties, can you fix that?

viirya · 2018-05-22T13:33:00Z

@cloud-fan Fixed. Thanks.

SparkQA · 2018-05-22T17:28:15Z

Test build #90965 has finished for PR 21106 at commit 5527595.

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

cloud-fan · 2018-05-23T14:41:19Z

thanks, merging to master!

Add the base of fallback logic fo UnsafeProjection.
fe2a1cd

hvanhovell reviewed Apr 19, 2018
View reviewed changes

Address comment.
36f90cf

Unify with UnsafeProjectionCreator.
68213fb

viirya added 3 commits May 6, 2018 12:54

Add a SQL config to control fallback mode under test.
6cdaec4

Merge remote-tracking branch 'upstream/master' into SPARK-23711
32e2766

Add test.
aeef468

viirya commented May 6, 2018
View reviewed changes

Don't access SQLConf on task execution.
1f5cc17

viirya force-pushed the SPARK-23711 branch from 06cc8cc to 1f5cc17Compare May 7, 2018 13:41

Simplify test suite.
8c56ae4

cloud-fan reviewed May 17, 2018
View reviewed changes

Don't use hardcoded configs.
5a735af

viirya added 2 commits May 22, 2018 03:30

Merge remote-tracking branch 'upstream/master' into SPARK-23711
5569646

Simplify SQL conf getting.
ed525f6

kiszk reviewed May 22, 2018
View reviewed changes

For comment.
c9ec817

Task context should not have null properties.
5527595

asfgit closed this in a40ffc6May 23, 2018

viirya deleted the SPARK-23711 branch December 27, 2023 18:21

Uh oh!

Conversation

viirya commented Apr 19, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

viirya commented Apr 19, 2018

Uh oh!

hvanhovellApr 19, 2018• 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!

viiryaApr 20, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 19, 2018

Uh oh!

SparkQA commented Apr 20, 2018

Uh oh!

viirya commented Apr 20, 2018

Uh oh!

SparkQA commented Apr 20, 2018

Uh oh!

viirya commented Apr 20, 2018

Uh oh!

SparkQA commented Apr 20, 2018

Uh oh!

viirya commented Apr 23, 2018

Uh oh!

hvanhovell commented Apr 23, 2018

Uh oh!

cloud-fan commented Apr 23, 2018

Uh oh!

viirya commented Apr 25, 2018

Uh oh!

cloud-fan commented Apr 26, 2018

Uh oh!

SparkQA commented Apr 26, 2018

Uh oh!

viirya commented Apr 27, 2018

Uh oh!

viirya commented May 3, 2018

Uh oh!

viirya commented May 6, 2018

Uh oh!

SparkQA commented May 6, 2018

Uh oh!

SparkQA commented May 6, 2018

Uh oh!

viiryaMay 6, 2018• 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!

SparkQA commented May 7, 2018

Uh oh!

SparkQA commented May 7, 2018

Uh oh!

cloud-fanMay 17, 2018

Choose a reason for hiding this comment

Uh oh!

cloud-fanMay 17, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 17, 2018

Uh oh!

SparkQA commented May 17, 2018

Uh oh!

SparkQA commented May 18, 2018

Uh oh!

viirya commented May 18, 2018

Uh oh!

cloud-fan commented May 18, 2018

Uh oh!

rednaxelafx commented May 18, 2018

Uh oh!

SparkQA commented May 18, 2018

Uh oh!

viirya commented May 22, 2018

Uh oh!

viirya commented Apr 19, 2018•
edited
Loading

hvanhovellApr 19, 2018•
edited
Loading

viiryaMay 6, 2018•
edited
Loading