Skip to content

[SPARK-34596][SPARK-34607][SQL][FOLLOWUP][TESTS][3.1] Add PlanTest. testFallback and use it #31764

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

Closed
dongjoon-hyun wants to merge 2 commits into from
Closed

[SPARK-34596][SPARK-34607][SQL][FOLLOWUP][TESTS][3.1] Add PlanTest. testFallback and use it #31764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a5a015
Use fallback
dongjoon-hyun Mar 6, 2021
ed5c3c0
fix cast suite
dongjoon-hyun Mar 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions ...talyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,8 @@ class ExpressionEncoderSuite extends CodegenInterpretedPlanTest with AnalysisTes
OuterScopes.addOuterScope(MalformedClassObject)
encodeDecodeTest(
MalformedClassObject.MalformedNameExample(42),
"nested Scala class should work")
"nested Scala class should work",
useFallback = true)
Copy link
Member

@maropu maropu Mar 8, 2021

Choose a reason for hiding this comment

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

How about leaving some comments here about why we need the fallback based on the Kris investigation? #31709 (comment)

}

object OuterLevelWithVeryVeryVeryLongClassName1 {
Expand Down Expand Up @@ -284,7 +285,8 @@ class ExpressionEncoderSuite extends CodegenInterpretedPlanTest with AnalysisTes
.OuterLevelWithVeryVeryVeryLongClassName19
.OuterLevelWithVeryVeryVeryLongClassName20
.MalformedNameExample(42),
"deeply nested Scala class should work")
"deeply nested Scala class should work",
useFallback = true)
}

productTest(PrimitiveData(1, 1, 1, 1, 1, 1, true))
Expand Down Expand Up @@ -555,8 +557,9 @@ class ExpressionEncoderSuite extends CodegenInterpretedPlanTest with AnalysisTes

private def encodeDecodeTest[T : ExpressionEncoder](
input: T,
testName: String): Unit = {
testAndVerifyNotLeakingReflectionObjects(s"encode/decode for $testName: $input") {
testName: String,
useFallback: Boolean = false): Unit = {
testAndVerifyNotLeakingReflectionObjects(s"encode/decode for $testName: $input", useFallback) {
val encoder = implicitly[ExpressionEncoder[T]]

// Make sure encoder is serializable.
Expand Down Expand Up @@ -650,9 +653,16 @@ class ExpressionEncoderSuite extends CodegenInterpretedPlanTest with AnalysisTes
r
}

private def testAndVerifyNotLeakingReflectionObjects(testName: String)(testFun: => Any): Unit = {
test(testName) {
verifyNotLeakingReflectionObjects(testFun)
private def testAndVerifyNotLeakingReflectionObjects(
testName: String, useFallback: Boolean = false)(testFun: => Any): Unit = {
if (useFallback) {
testFallback(testName) {
verifyNotLeakingReflectionObjects(testFun)
}
} else {
test(testName) {
verifyNotLeakingReflectionObjects(testFun)
}
}
}
}
2 changes: 1 addition & 1 deletion sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,7 +961,7 @@ abstract class AnsiCastSuiteBase extends CastSuiteBase {
}

test("ANSI mode: cast string to timestamp with parse error") {
val activeConf = conf
val activeConf = conf.clone()
Copy link
Contributor

@cloud-fan cloud-fan Mar 8, 2021

Choose a reason for hiding this comment

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

do we have to clone the conf here?

Copy link
Member

@kiszk kiszk Mar 8, 2021

Choose a reason for hiding this comment

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

agree, I thought that this was a tentative workaround.

Copy link
Member

@kiszk kiszk Mar 8, 2021

Choose a reason for hiding this comment

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

@dongjoon-hyun Can #31775 allow us to revert this?

Copy link
Member

@viirya viirya Mar 8, 2021

Choose a reason for hiding this comment

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

I think so.

new ParVector(ALL_TIMEZONES.toVector).foreach { zid =>
def checkCastWithParseError(str: String): Unit = {
checkExceptionInExpression[DateTimeException](
Expand Down
9 changes: 9 additions & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,15 @@ trait CodegenInterpretedPlanTest extends PlanTest {
super.test(testName + " (interpreted path)", testTags: _*)(testFun)(pos)
}
}

protected def testFallback(
Copy link
Contributor

@cloud-fan cloud-fan Mar 8, 2021

Choose a reason for hiding this comment

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

The current test framework assumes that codegen and non-codegen should have consistent behaviors, while this codegen bug breaks the assumption. The test case fails with codegen but passes with non-codegen.

+1 to add this for such test cases. One thing I'm curious about is why this test works in master...

Copy link
Member

@maropu maropu Mar 8, 2021

Choose a reason for hiding this comment

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

+1 to add it, too.

testName: String,
testTags: Tag*)(testFun: => Any)(implicit pos: source.Position): Unit = {
val codegenMode = CodegenObjectFactoryMode.FALLBACK.toString
withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) {
super.test(testName, testTags: _*)(testFun)(pos)
Copy link
Member

@maropu maropu Mar 8, 2021

Choose a reason for hiding this comment

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

nit: How about adding an additional message here, e.g., testName +" (codegen fallback mode)"?

}
Comment on lines +59 to +61
Copy link
Member

@viirya viirya Mar 6, 2021

Choose a reason for hiding this comment

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

Maybe we should run twice, one for FALLBACK and one for NO_CODEGEN.

FALLBACK will run codegen path first and fallback to interpreted mode if codegen fails. So interpreted mode may not run if codegen mode successes.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Mar 6, 2021

Choose a reason for hiding this comment

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

That sounds like another use case. Technically, this testFallback is designed to the default mode of this configuration (which is the default of Apache Spark runtime in non-testing mode) instead of a specific mode interpreted mode or codeine.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Mar 6, 2021

Choose a reason for hiding this comment

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

Maybe, do you want to add testCodegenFail?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Mar 6, 2021

Choose a reason for hiding this comment

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

Hmm. At the second thought, I understand what was your suggestion. So, your suggestion is to check the occurrence of fallback, right? Then, it makes sense.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Mar 6, 2021

Choose a reason for hiding this comment

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

I'll update my PR in this afternoon~

Copy link
Member

@viirya viirya Mar 6, 2021

Choose a reason for hiding this comment

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

test runs the test code twice, one for codegen and one for interpreted. It is for test coverage.

If testFallback only runs for fallback mode, then it might only runs codegen and skips interpreted mode if codegen successes. So the test coverage is less, I think.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Mar 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, @viirya . I tried to revise, but it seems strange to me.

test runs the test code twice, one for codegen and one for interpreted. It is for test coverage.
If testFallback only runs for fallback mode, then it might only runs codegen and skips interpreted mode if codegen successes. So the test coverage is less, I think.

testFallback is not aiming to replace test in line 41. Instead, testFallback is added because PlanTest bans all derived classes from testing FALLBACK mode. Before this PR, there is no way to test FALLBACK mode.

As we know, CODEGEN_FACTORY_MODE has three modes: FALLBACK, CODEGEN_ONLY, NO_CODEGEN.
As we guess in the name, testFallback, this test is specifically for FALLBACK mode and is added for some test cases which works only at FALLBACK mode as @rednaxelafx described.

So, testFallback doesn't aim to mean (1) CODEGEN_ONLY should fails and (2) NO_CODEGEN should passed. If you want to test this (both (1) and (2)), we should make another test function like testCodegenFailNoCodegenPass.

Copy link
Member

@viirya viirya Mar 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

FALLBACK mode is not a special path other than codegen and interpreted path. Under FALLBACK mode, Spark runs first codegen path and then interpreted path if codegen fails.

So it sounds weird that some test cases works only at FALLBACK mode, so we only need to run it with FALLBACK mode. Doesn't it just mean the test cases may fail under codegen but can success under interpreted?

Wrapping the test function with FALLBACK config, means we may only run the test in codegen path, if codegen path successes. It skips interpreted path in the case.

So, it may unintentionally avoid the test coverage of interpreted path. That means, if we use testFallback, it might only test codegen path if codegen path successes. Interpreted path will not be tested for the case.

The current testFallback only makes sense if we only want to make sure the test case works, no matter it is codegen or interpreted mode. If this is the purpose, then it is fine.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Mar 8, 2021

Choose a reason for hiding this comment

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

Hi, @viirya . It seems that you missed @rednaxelafx 's comment, #31709 (comment) .

So it sounds weird that some test cases works only at FALLBACK mode, so we only need to run it with FALLBACK mode. Doesn't it just mean the test cases may fail under codegen but can success under interpreted?

}
}

/**
Expand Down