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

HeartSaVioR · 2021-02-02T08:13:28Z

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-02T08:14:58Z

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

+      val pathsInLocation = location.get.substring(
+        location.get.indexOf('[') + 1, location.get.indexOf(']')).split(", ").toSeq
+
+      // If the temp path length is less than (stop appending threshold - 1), say, 100 - 1 = 99,


I intentionally excluded the thing on closing bracket (']'), but the closing bracket will be added in any way, regardless of the current location string length. This was also not accounted as well.

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

cc.ing @gengliangwang @cloud-fan @HyukjinKwon @maropu who are author/reviewers of #28610

SparkQA · 2021-02-02T09:24:49Z

Test build #134770 has started for PR 31435 at commit 36943c6.

gengliangwang · 2021-02-02T12:07:38Z

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

@@ -137,9 +137,24 @@ class DataSourceScanExecRedactionSuite extends DataSourceScanRedactionTest {
      assert(location.isDefined)
      // The location metadata should at least contain one path
      assert(location.get.contains(paths.head))
-      // If the temp path length is larger than 100, the metadata length should not exceed
-      // twice of the length; otherwise, the metadata length should be controlled within 200.
-      assert(location.get.length < Math.max(paths.head.length, 100) * 2)


You are right, this can be flaky.

gengliangwang · 2021-02-02T12:08:20Z

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

+      // path.
+      // (Note we apply subtraction with 1 to count start bracket '['.)
+      if (paths.head.length < 99) {
+        assert(pathsInLocation.size >= 2)


We still need to check that the paths are truncated here.

I'm not sure I understand. Could you please elaborate?

We don't truncate the path itself, right? I think it's also something to be fixed (I'd rather want to see path being truncated with ellipses (...) instead of not adding and leaving it as it is.) but it's more likely bigger fix which may worth another fix instead of test fix.

If you meant counting the number of paths or something for edge-case, dealing with another UT would be easier, like we could simply add edge-cases there in this PR. (And that UT already tests basic functionality.)

I mean simply checking

assert(pathsInLocation.size < paths.size)

But this is a minor comment. You can ignore it since there is UT for it already.

Yeah makes sense, but let's leave it as it is, as I think I'd propose the another form which would make clear how many elements they are instead of only showing available paths and don't say there're more.

maropu · 2021-02-02T12:24:40Z

Nice catch!

HeartSaVioR · 2021-02-02T13:40:33Z

retest this, please

HeartSaVioR · 2021-02-02T13:43:00Z

Just retriggered Github Action as well.

SparkQA · 2021-02-02T14:57:57Z

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

SparkQA · 2021-02-02T15:07:33Z

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

SparkQA · 2021-02-02T16:18:52Z

Test build #134782 has finished for PR 31435 at commit 36943c6.

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

HeartSaVioR · 2021-02-02T22:33:38Z

Merging to master/3.1.

…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 #31435 from HeartSaVioR/SPARK-34326. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com> (cherry picked from commit 6386602) Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>

HeartSaVioR · 2021-02-02T22:38:27Z

Thanks all for the quick reviews!

HyukjinKwon · 2021-02-03T03:32:18Z

Hm, the tests started to fail:

[info] - SPARK-31793: FileSourceScanExec metadata should contain limited file paths *** FAILED *** (265 milliseconds)
[info]   1 was not greater than or equal to 2 (DataSourceScanExecRedactionSuite.scala:154)
[info]   org.scalatest.exceptions.TestFailedException:
[info]   at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
[info]   at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
[info]   at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info]   at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info]   at org.apache.spark.sql.execution.DataSourceScanExecRedactionSuite.$anonfun$new$9(DataSourceScanExecRedactionSuite.scala:154)
[info]   at org.apache.spark.sql.execution.DataSourceScanExecRedactionSuite.$anonfun$new$9$adapted(DataSourceScanExecRedactionSuite.scala:123)
[info]   at org.apache.spark.sql.catalyst.plans.SQLHelper.withTempPath(SQLHelper.scala:69)
[info]   at org.apache.spark.sql.catalyst.plans.SQLHelper.withTempPath$(SQLHelper.scala:66)
[info]   at org.apache.spark.sql.QueryTest.withTempPath(QueryTest.scala:34)
[info]   at org.apache.spark.sql.execution.DataSourceScanExecRedactionSuite.$anonfun$new$8(DataSourceScanExecRedactionSuite.scala:123)
[info]   at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)

https://github.com/apache/spark/runs/1818352990
https://github.com/apache/spark/runs/1818908589

@HeartSaVioR, please let me revert this for now since it targeted to fix the test but it started to fail in any event.

HeartSaVioR · 2021-02-03T03:36:29Z

Ah thanks for the check. Looks odd... I'll check it.

HeartSaVioR · 2021-02-03T05:09:46Z

Raised a new PR #31449

HeartSaVioR · 2021-02-03T05:09:49Z

Raised a new PR #31449

…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 apache#31435 from HeartSaVioR/SPARK-34326. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>

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

36943c6

…e length of temp path

HeartSaVioR commented Feb 2, 2021

View reviewed changes

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

HyukjinKwon approved these changes Feb 2, 2021

View reviewed changes

gengliangwang reviewed Feb 2, 2021

View reviewed changes

maropu approved these changes Feb 2, 2021

View reviewed changes

gengliangwang approved these changes Feb 2, 2021

View reviewed changes

HeartSaVioR closed this in 6386602 Feb 2, 2021

HeartSaVioR deleted the SPARK-34326 branch February 2, 2021 22:38

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 #31435

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

HeartSaVioR commented Feb 2, 2021

HeartSaVioR Feb 2, 2021 •

edited

Loading

HeartSaVioR commented Feb 2, 2021

SparkQA commented Feb 2, 2021

gengliangwang Feb 2, 2021

gengliangwang Feb 2, 2021

HeartSaVioR Feb 2, 2021 •

edited

Loading

gengliangwang Feb 2, 2021

HeartSaVioR Feb 2, 2021

maropu commented Feb 2, 2021

HeartSaVioR commented Feb 2, 2021

HeartSaVioR commented Feb 2, 2021

SparkQA commented Feb 2, 2021

SparkQA commented Feb 2, 2021

SparkQA commented Feb 2, 2021

HeartSaVioR commented Feb 2, 2021

HeartSaVioR commented Feb 2, 2021

HyukjinKwon commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

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

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

Conversation

HeartSaVioR commented Feb 2, 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 2, 2021 • edited Loading

Choose a reason for hiding this comment

HeartSaVioR commented Feb 2, 2021

SparkQA commented Feb 2, 2021

gengliangwang Feb 2, 2021

Choose a reason for hiding this comment

gengliangwang Feb 2, 2021

Choose a reason for hiding this comment

HeartSaVioR Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

gengliangwang Feb 2, 2021

Choose a reason for hiding this comment

HeartSaVioR Feb 2, 2021

Choose a reason for hiding this comment

maropu commented Feb 2, 2021

HeartSaVioR commented Feb 2, 2021

HeartSaVioR commented Feb 2, 2021

SparkQA commented Feb 2, 2021

SparkQA commented Feb 2, 2021

SparkQA commented Feb 2, 2021

HeartSaVioR commented Feb 2, 2021

HeartSaVioR commented Feb 2, 2021

HyukjinKwon commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HeartSaVioR commented Feb 3, 2021

HeartSaVioR Feb 2, 2021 •

edited

Loading

HeartSaVioR Feb 2, 2021 •

edited

Loading