[SPARK-34326][CORE][SQL] Fix UTs added in SPARK-31793 depending on the length of temp path #31449

HeartSaVioR · 2021-02-03T04:45:12Z

What changes were proposed in this pull request?

This PR proposes to fix the UTs being added in SPARK-31793, so that all things contributing the length limit are properly accounted.

Why are the changes needed?

The test DataSourceScanExecRedactionSuite.SPARK-31793: FileSourceScanExec metadata should contain limited file paths is failing conditionally, depending on the length of the temp directory.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Modified UTs explain the missing points, which also do the test.

…e length of temp path

HeartSaVioR · 2021-02-03T04:46:33Z

sql/core/src/test/scala/org/apache/spark/sql/execution/DataSourceScanExecRedactionSuite.scala

@@ -122,24 +123,45 @@ class DataSourceScanExecRedactionSuite extends DataSourceScanRedactionTest {
  test("SPARK-31793: FileSourceScanExec metadata should contain limited file paths") {
    withTempPath { path =>
      val dir = path.getCanonicalPath
+
+      // create a sub-directory with long name so that each root path will always exceed the limit


Let's just ensure we test on path truncation; other cases should be tested against UtilsSuite.

HeartSaVioR · 2021-02-03T04:53:17Z

Sorry there looked to be some case I haven't taken care of. (The log file doesn't say about the actual path, but I'm suspecting the difference between path without schema vs with schema - file:///.)

Instead of struggling with thinking of all possible cases, I think it's more appropriate if we ensure the test always verifies the case path truncation happens and only one path is accepted. Other cases are covered by UtilsSuite.

Please take a look at this again. Thanks!

SparkQA · 2021-02-03T05:49:55Z

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

SparkQA · 2021-02-03T05:54:52Z

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

SparkQA · 2021-02-03T07:44:16Z

Test build #134807 has finished for PR 31449 at commit 5f37003.

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

maropu · 2021-02-03T08:01:46Z

sql/core/src/test/scala/org/apache/spark/sql/execution/DataSourceScanExecRedactionSuite.scala

+      // create a sub-directory with long name so that each root path will always exceed the limit
+      // this is to ensure we always test the case for the path truncation
+      // 110 = limit 100 + margin 10 to clearly avoid edge case
+      val dataDir = if (dir.length >= 110) {


nit comment; for test simplicity, how about always creating a new dir with fixed-length long name (e.g., 110?) instead of checking the length (if (dir.length >= 110))?

val tooLongDirName = Random.alphanumeric.take(110).toList.mkString val f = new File(path, tooLongDirName) f.mkdir()

I'm not 100% sure about the possible glitch on long long path (like exceeding 255 chars), but I agree that should be simpler if there's no problem on the length of path. Do you have any idea on this?

Ah I see. If so, the current one looks fine. Could you leave some comments there about the limit? I didn't know it and it seems the max name length is 255 and the max path length is 4096? (I googled it)

I don't know about the actual problem; probably just worrying too much (that's why I said "not 100% sure"). Let's simplify as you suggested and revisit if we trigger any glitch.

maropu · 2021-02-03T08:03:43Z

To check if we can resolve the flakiness, could you run GA tests multiple times (3~4?) before merging it?

HeartSaVioR · 2021-02-03T08:05:02Z

Both Jenkins and GA are happy. Will retrigger both again. (Leaving the fact as comment because GA doesn't leave the build result to the comment.)

HeartSaVioR · 2021-02-03T08:05:14Z

retest this, please

SparkQA · 2021-02-03T08:23:04Z

Test build #134822 has started for PR 31449 at commit 5f37003.

SparkQA · 2021-02-03T09:10:45Z

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

SparkQA · 2021-02-03T09:41:55Z

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

SparkQA · 2021-02-03T10:07:54Z

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

SparkQA · 2021-02-03T10:37:14Z

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

SparkQA · 2021-02-03T11:28:49Z

Test build #134823 has finished for PR 31449 at commit c6cd289.

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

gengliangwang

LGTM

HeartSaVioR · 2021-02-03T14:13:18Z

OK second run for Jenkins / GA passed. I'll execute another run and check tomorrow morning, and merge if the next run also passes.

HeartSaVioR · 2021-02-03T14:13:31Z

retest this, please

SparkQA · 2021-02-03T17:17:45Z

Test build #134838 has finished for PR 31449 at commit c6cd289.

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

HeartSaVioR · 2021-02-03T19:19:23Z

Jenkins failed, but not from related UTs. Let me retrigger one. (GA is still running, looks like the workflow was started less than 2 hours?)

HeartSaVioR · 2021-02-03T19:20:13Z

retest this, please

HeartSaVioR · 2021-02-03T19:45:58Z

GA failed but also not from related UT. retriggering...

SparkQA · 2021-02-03T22:43:42Z

Test build #134846 has finished for PR 31449 at commit c6cd289.

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

HeartSaVioR · 2021-02-03T23:45:11Z

OK Jenkins/GA passed. Merging...

…e length of temp path ### What changes were proposed in this pull request? This PR proposes to fix the UTs being added in SPARK-31793, so that all things contributing the length limit are properly accounted. ### Why are the changes needed? The test `DataSourceScanExecRedactionSuite.SPARK-31793: FileSourceScanExec metadata should contain limited file paths` is failing conditionally, depending on the length of the temp directory. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Modified UTs explain the missing points, which also do the test. Closes #31449 from HeartSaVioR/SPARK-34326-v2. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com> (cherry picked from commit 44dcf00) Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>

HeartSaVioR · 2021-02-03T23:47:44Z

Thanks for the quick reviews! Merged into master/branch-3.1.

HeartSaVioR · 2021-02-03T23:49:43Z

Will monitor the GA run for master/branch-3.1.

master: https://github.com/apache/spark/runs/1826766295
branch-3.1: https://github.com/apache/spark/runs/1826767307

HyukjinKwon · 2021-02-04T01:23:43Z

Nice, thanks for fixing it @HeartSaVioR

maropu · 2021-02-05T00:55:06Z

Thanks~, @HeartSaVioR

[SPARK-34326][CORE][SQL] Fix UTs added in SPARK-31793 depending on th…

5f37003

…e length of temp path

HeartSaVioR commented Feb 3, 2021

View reviewed changes

HeartSaVioR requested review from HyukjinKwon, gengliangwang and maropu and removed request for HyukjinKwon February 3, 2021 04:48

HeartSaVioR mentioned this pull request Feb 3, 2021

[SPARK-34326][CORE][SQL] Fix UTs added in SPARK-31793 depending on the length of temp path #31435

Closed

github-actions bot added CORE SQL labels Feb 3, 2021

maropu reviewed Feb 3, 2021

View reviewed changes

Simplify the test

c6cd289

gengliangwang approved these changes Feb 3, 2021

View reviewed changes

HeartSaVioR closed this in 44dcf00 Feb 3, 2021

HeartSaVioR deleted the SPARK-34326-v2 branch February 4, 2021 01:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-34326][CORE][SQL] Fix UTs added in SPARK-31793 depending on the length of temp path #31449

[SPARK-34326][CORE][SQL] Fix UTs added in SPARK-31793 depending on the length of temp path #31449

HeartSaVioR commented Feb 3, 2021

HeartSaVioR Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

maropu Feb 3, 2021

HeartSaVioR Feb 3, 2021

maropu Feb 3, 2021

HeartSaVioR Feb 3, 2021 •

edited

Loading

maropu commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

gengliangwang left a comment

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

SparkQA commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021 •

edited

Loading

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

SparkQA commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HyukjinKwon commented Feb 4, 2021

maropu commented Feb 5, 2021

[SPARK-34326][CORE][SQL] Fix UTs added in SPARK-31793 depending on the length of temp path #31449

[SPARK-34326][CORE][SQL] Fix UTs added in SPARK-31793 depending on the length of temp path #31449

Conversation

HeartSaVioR commented Feb 3, 2021

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?

HeartSaVioR Feb 3, 2021

Choose a reason for hiding this comment

HeartSaVioR commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

maropu Feb 3, 2021

Choose a reason for hiding this comment

HeartSaVioR Feb 3, 2021

Choose a reason for hiding this comment

maropu Feb 3, 2021

Choose a reason for hiding this comment

HeartSaVioR Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

maropu commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

SparkQA commented Feb 3, 2021

gengliangwang left a comment

Choose a reason for hiding this comment

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

SparkQA commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021 • edited Loading

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

SparkQA commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HyukjinKwon commented Feb 4, 2021

maropu commented Feb 5, 2021

HeartSaVioR Feb 3, 2021 •

edited

Loading

HeartSaVioR commented Feb 3, 2021 •

edited

Loading