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

dongjoon-hyun · 2021-03-06T07:46:48Z

What changes were proposed in this pull request?

This PR aims to add testFallback function to PlanTest.

This PR is created for branch-3.1 because it is broken for now. However, all relevant patches are in master and branch-3.0, too. So, after having a complete patch, I'll make two more PRs to master/branch-3.0.

Why are the changes needed?

Some test cases only work in CodegenObjectFactoryMode.FALLBACK mode while PlanTest doesn't support it because it always overrides the test case and run twice with CODEGEN_ONLY and NO_CODEGEN.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the CIs.

dongjoon-hyun · 2021-03-06T07:51:47Z

cc @cloud-fan , @rednaxelafx , @maropu and @viirya

kiszk · 2021-03-06T08:11:26Z

Thank you for creating a WIP. Could you please clarify "make two more PRs"?

rednaxelafx · 2021-03-06T08:48:18Z

Thank you very much for helping with the follow-up on this @dongjoon-hyun ! Really appreciate it!
Your proposed fix here look nice.

I've been debugging this on my side as well. There's gotta be something screwing up with the failure (or the lack of failure on master/3.0)...

SparkQA · 2021-03-06T08:50:26Z

Test build #135824 has finished for PR 31764 at commit 1a5a015.

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

SparkQA · 2021-03-06T09:19:15Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40406/

SparkQA · 2021-03-06T09:48:58Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40406/

dongjoon-hyun · 2021-03-06T15:30:26Z

Thank you, @kiszk and @rednaxelafx .

To @kiszk . I need to make the same PR to master and branch-3.1 in order to pass the CI.

Thank you for creating a WIP. Could you please clarify "make two more PRs"?

kiszk · 2021-03-06T15:33:26Z

Thanks. I see. One for master. One for branch-3.1.

SparkQA · 2021-03-06T18:11:04Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40415/

viirya · 2021-03-06T18:31:39Z

This is WIP because the tests still fail even in Fallback mode.

This is not anymore?

SparkQA · 2021-03-06T18:43:49Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40415/

dongjoon-hyun · 2021-03-06T20:31:14Z

Yes, this is ready for review, @viirya .

viirya · 2021-03-06T20:36:05Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala

+    withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) {
+      super.test(testName, testTags: _*)(testFun)(pos)
+    }


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.

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.

Maybe, do you want to add testCodegenFail?

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.

I'll update my PR in this afternoon~

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.

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.

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.

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?

dongjoon-hyun · 2021-03-06T20:50:32Z

I created a PR for master branch.

[SPARK-34696][SQL][TESTS] Fix CodegenInterpretedPlanTest to generate correct test cases #31766

SparkQA · 2021-03-06T21:47:47Z

Test build #135833 has finished for PR 31764 at commit ed5c3c0.

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

cloud-fan · 2021-03-08T04:50:10Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala


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


do we have to clone the conf here?

agree, I thought that this was a tentative workaround.

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

I think so.

cloud-fan · 2021-03-08T04:58:40Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala

    }
  }
+
+  protected def testFallback(


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...

+1 to add it, too.

maropu · 2021-03-08T05:36:01Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala

+      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)


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

maropu · 2021-03-08T05:46:15Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala

      MalformedClassObject.MalformedNameExample(42),
-      "nested Scala class should work")
+      "nested Scala class should work",
+      useFallback = true)


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

dongjoon-hyun · 2021-03-08T06:56:15Z

Hi, All.

Currently, this PR is related two stuff.

testFallback is designed to clarify the goal of test case.
Recovering branch-3.1.

For (2), it seems that I found another solution. I'll make another PR for that.

dongjoon-hyun · 2021-03-08T07:18:48Z

I made a PR for recovering branch-3.1.

[SPARK-34660][TESTS][3.1] Don't use ParVector with withExistingConf which is not thread-safe #31775

kiszk · 2021-03-08T07:25:48Z

@dongjoon-hyun Should we move #31775 for branch-3.1? Then, will you close this PR?

dongjoon-hyun · 2021-03-08T07:33:25Z

No~, @kiszk . testFallback is required for the two test cases of SPARK-34596 and SPARK34607 because we had better explicitly use testFallback for those test case whose behavior is not runnable in both modes.

I believe this is a kind of refactoring of PlanTest to clarify the purpose and intention of the test cases.

kiszk · 2021-03-08T07:34:28Z

I see.

dongjoon-hyun · 2021-03-08T07:35:11Z

Anyway, I'll make this as a draft for now.

Use fallback

1a5a015

github-actions bot added the SQL label Mar 6, 2021

dongjoon-hyun marked this pull request as draft March 6, 2021 07:47

dongjoon-hyun mentioned this pull request Mar 6, 2021

[SPARK-34596][SQL] Use Utils.getSimpleName to avoid hitting Malformed class name in NewInstance.doGenCode #31709

Closed

dongjoon-hyun requested a review from maropu March 6, 2021 07:51

fix cast suite

ed5c3c0

dongjoon-hyun changed the title ~~[WIP] Use fallback~~ [SPARK-34596][SQL][FOLLOWUP][TESTS] Add testFallback to PlanTest and use it properly Mar 6, 2021

dongjoon-hyun changed the title ~~[SPARK-34596][SQL][FOLLOWUP][TESTS] Add testFallback to PlanTest and use it properly~~ [SPARK-34596][SPARK-34607][SQL][FOLLOWUP][TESTS] Add testFallback to PlanTest and use it properly Mar 6, 2021

dongjoon-hyun changed the title ~~[SPARK-34596][SPARK-34607][SQL][FOLLOWUP][TESTS] Add testFallback to PlanTest and use it properly~~ [SPARK-34596][SPARK-34607][SQL][FOLLOWUP][TESTS] Add testFallback to PlanTest and use it Mar 6, 2021

dongjoon-hyun changed the title ~~[SPARK-34596][SPARK-34607][SQL][FOLLOWUP][TESTS] Add testFallback to PlanTest and use it~~ [SPARK-34596][SPARK-34607][SQL][FOLLOWUP][TESTS] Add PlanTest. testFallback and use it Mar 6, 2021

dongjoon-hyun marked this pull request as ready for review March 6, 2021 17:56

dongjoon-hyun changed the title ~~[SPARK-34596][SPARK-34607][SQL][FOLLOWUP][TESTS] Add PlanTest. testFallback and use it~~ [SPARK-34596][SPARK-34607][SQL][FOLLOWUP][TESTS][3.1] Add PlanTest. testFallback and use it Mar 6, 2021

viirya reviewed Mar 6, 2021

View reviewed changes

cloud-fan reviewed Mar 8, 2021

View reviewed changes

maropu reviewed Mar 8, 2021

View reviewed changes

dongjoon-hyun marked this pull request as draft March 8, 2021 07:35

dongjoon-hyun closed this Mar 10, 2021

dongjoon-hyun deleted the wip branch March 10, 2021 07:05

dongjoon-hyun mentioned this pull request Mar 10, 2021

[SPARK-34696][SQL][TESTS] Fix CodegenInterpretedPlanTest to generate correct test cases #31766

Closed

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

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

Uh oh!

Conversation

dongjoon-hyun commented Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

dongjoon-hyun commented Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiszk commented Mar 6, 2021

Uh oh!

rednaxelafx commented Mar 6, 2021

Uh oh!

SparkQA commented Mar 6, 2021

Uh oh!

SparkQA commented Mar 6, 2021

Uh oh!

SparkQA commented Mar 6, 2021

Uh oh!

dongjoon-hyun commented Mar 6, 2021

Uh oh!

kiszk commented Mar 6, 2021

Uh oh!

SparkQA commented Mar 6, 2021

Uh oh!

viirya commented Mar 6, 2021

Uh oh!

SparkQA commented Mar 6, 2021

Uh oh!

dongjoon-hyun commented Mar 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Mar 7, 2021 • 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!

viirya Mar 7, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Mar 6, 2021

Uh oh!

SparkQA commented Mar 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Mar 8, 2021

Uh oh!

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

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

dongjoon-hyun commented Mar 6, 2021 •

edited

Loading

dongjoon-hyun commented Mar 6, 2021 •

edited

Loading

dongjoon-hyun Mar 7, 2021 •

edited

Loading

viirya Mar 7, 2021 •

edited

Loading

dongjoon-hyun commented Mar 8, 2021 •

edited

Loading