Skip to content
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.

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

[SPARK-8115] [SQL] Remove TestData #7406

Closed
wants to merge 38 commits into from
Closed

[SPARK-8115] [SQL] Remove TestData #7406

wants to merge 38 commits into from

Conversation

BenFradet
Copy link
Contributor

This PR aims to bring the test datasets closer to the test suites.

@SparkQA
Copy link

SparkQA commented Jul 14, 2015

Test build #37271 has finished for PR 7406 at commit 2ab06a4.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public static class TestData implements Serializable
    • public static class TestData2 implements Serializable
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)
    • case class ArrayData(data: Seq[Int], nestedData: Seq[Seq[Int]])
    • case class MapData(data: scala.collection.Map[Int, String])
    • case class ArrayData(data: Seq[Int], nestedData: Seq[Seq[Int]])
    • case class MapData(data: scala.collection.Map[Int, String])
    • case class TestData(key: Int, value: String)
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)

@SparkQA
Copy link

SparkQA commented Jul 14, 2015

Test build #37274 has finished for PR 7406 at commit 1635564.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public static class TestData implements Serializable
    • public static class TestData2 implements Serializable
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)
    • case class ArrayData(data: Seq[Int], nestedData: Seq[Seq[Int]])
    • case class MapData(data: scala.collection.Map[Int, String])
    • case class ArrayData(data: Seq[Int], nestedData: Seq[Seq[Int]])
    • case class MapData(data: scala.collection.Map[Int, String])
    • case class TestData(key: Int, value: String)
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)

@rxin
Copy link
Contributor

rxin commented Jul 15, 2015

@BenFradet thanks for doing this. I think this is fairly useful & important, but the best time to merge this is during our QA period for 1.5.0, i.e. in the first 2 weeks of Aug, to avoid creating a lot of conflicts.

Can we revisit this after Aug 1?

@BenFradet
Copy link
Contributor Author

Sure, no problem.
On 15 Jul 2015 8:05 pm, "Reynold Xin" notifications@github.com wrote:

@BenFradet https://github.com/BenFradet thanks for doing this. I think
this is fairly useful & important, but the best time to merge this is
during our QA period for 1.5.0, i.e. in the first 2 weeks of Aug, to avoid
creating a lot of conflicts.

Can we revisit this after Aug 1?


Reply to this email directly or view it on GitHub
#7406 (comment).

@rxin
Copy link
Contributor

rxin commented Aug 3, 2015

@BenFradet After we've cut the release branch for 1.5, it would be great to bring this up-to-date so we can merge it. Thanks!

@BenFradet
Copy link
Contributor Author

Yup, will do later in the week.

@SparkQA
Copy link

SparkQA commented Aug 6, 2015

Test build #40074 has finished for PR 7406 at commit 7d47151.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public static class TestData implements Serializable
    • public static class TestData2 implements Serializable
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)
    • case class ArrayData(data: Seq[Int], nestedData: Seq[Seq[Int]])
    • case class MapData(data: scala.collection.Map[Int, String])
    • case class ArrayData(data: Seq[Int], nestedData: Seq[Seq[Int]])
    • case class MapData(data: scala.collection.Map[Int, String])
    • case class TestData(key: Int, value: String)
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)

@SparkQA
Copy link

SparkQA commented Aug 10, 2015

Test build #40276 has finished for PR 7406 at commit 2c96ada.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds the following public classes (experimental):
    • public static class TestData implements Serializable
    • public static class TestData2 implements Serializable
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)
    • case class ArrayData(data: Seq[Int], nestedData: Seq[Seq[Int]])
    • case class MapData(data: scala.collection.Map[Int, String])
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)
    • case class ArrayData(data: Seq[Int], nestedData: Seq[Seq[Int]])
    • case class ArrayData(data: Seq[Int], nestedData: Seq[Seq[Int]])
    • case class MapData(data: scala.collection.Map[Int, String])
    • case class TestData(key: Int, value: String)
    • case class TestData(key: Int, value: String)
    • case class ComplexData(m: Map[String, Int], s: TestData, a: Seq[Int], b: Boolean)

@BenFradet
Copy link
Contributor Author

I have no idea why both of those tests (this one and this one) are failing, much help needed cc @rxin.

Will rebase soon.

@rxin
Copy link
Contributor

rxin commented Aug 11, 2015

Thanks @BenFradet. Looks like @andrewor14 is working on this at the same time to also get rid of the singleton TestSQLContext in order for his own tests to pass.

#8111

He will incorporate your changes into that.

@BenFradet
Copy link
Contributor Author

Ok, thanks for keeping me in the loop.

@BenFradet
Copy link
Contributor Author

@rxin Should I close this then?

@rxin
Copy link
Contributor

rxin commented Aug 12, 2015

Actually it looks like @andrewor14's patch just moved TestData somewhere else to remove the singleton. Maybe we should still bring this one up to date once his merges ... :)

@andrewor14
Copy link
Contributor

Don't we want a common collection of test data shared among different tests? It helps reducing duplicate code.

@rxin
Copy link
Contributor

rxin commented Aug 12, 2015

No it is better to have the data closer to the test cases. Right now for
most test cases it is hard to know what the correct answers should be
because the input data is not defined in the test case (or even same file).

On Wednesday, August 12, 2015, andrewor14 notifications@github.com wrote:

Don't we want a common collection of test data shared among different
tests? It helps reducing duplicate code.


Reply to this email directly or view it on GitHub
#7406 (comment).

@BenFradet
Copy link
Contributor Author

@rxin @andrewor14

This patch doesnt really make sense with the introduction of SQLTestData in #8111, does it?

@SparkQA
Copy link

SparkQA commented Aug 19, 2015

Test build #41284 has finished for PR 7406 at commit 3a022ab.

  • This patch fails Scala style tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor

Hey @BenFradet, would you mind closing this PR for now? While I think that we may eventually remove TestData, this PR is going to be really merge-conflict prone and I think it would be easier for one of the committers to take it over and handle fixing the conflicts. If we do that, we'll be sure to credit your initial work here and use it as the foundation for our patch. Thanks!

@BenFradet BenFradet closed this Oct 16, 2015
@BenFradet
Copy link
Contributor Author

Sure, no problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants