[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[cloud-fan]

Github user cloud-fan commented on the issue:

https://github.com/apache/spark/pull/21568
  
thanks, merging to master!


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[cloud-fan]

Github user cloud-fan commented on the issue:

https://github.com/apache/spark/pull/21568
  
LGTM


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[AmplabJenkins]

Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/21568
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92857/
Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[AmplabJenkins]

Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/21568
  
Merged build finished. Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[SparkQA]

Github user SparkQA commented on the issue:

https://github.com/apache/spark/pull/21568
  
**[Test build #92857 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92857/testReport)**
 for PR 21568 at commit 
[`6f90e63`](https://github.com/apache/spark/commit/6f90e63b0ca2b929c2e6e8be6b090c4fe0bb9583).
 * This patch passes all tests.
 * This patch merges cleanly.
 * This patch adds no public classes.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[mgaido91]

Github user mgaido91 commented on the issue:

https://github.com/apache/spark/pull/21568
  
@cloud-fan @rxin I updated the PR in order to handle only the case in which 
we have the same result for different configs and the configs are specified in 
the SQL files.
In order to make clear which config caused a job error I added a ERROR 
message in the logs.

Do you all agree with this approach?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[SparkQA]

Github user SparkQA commented on the issue:

https://github.com/apache/spark/pull/21568
  
**[Test build #92857 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92857/testReport)**
 for PR 21568 at commit 
[`6f90e63`](https://github.com/apache/spark/commit/6f90e63b0ca2b929c2e6e8be6b090c4fe0bb9583).


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[AmplabJenkins]

Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/21568
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 

https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/847/
Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[AmplabJenkins]

Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/21568
  
Merged build finished. Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[rxin]

Github user rxin commented on the issue:

https://github.com/apache/spark/pull/21568
  
To me it is actually confusing to have the decimal one in there at all, by
defining a list of queries that are reused for different functional
testing. It is very easy to just ignore the subtle differences.

We are also risk over engineering this with only one use case.

On Tue, Jul 10, 2018 at 8:20 AM Wenchen Fan 
wrote:

> We can deal with the decimal test file specially if that's the only use
> case. For now I'd say the join test is more important and let's finish it
> first.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> , or 
mute
> the thread
> 

> .
>



---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[cloud-fan]

Github user cloud-fan commented on the issue:

https://github.com/apache/spark/pull/21568
  
We can deal with the decimal test file specially if that's the only use 
case. For now I'd say the join test is more important and let's finish it first.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[mgaido91]

Github user mgaido91 commented on the issue:

https://github.com/apache/spark/pull/21568
  
I am not sure it is a great idea to do only one of the 2 scenarios, if we 
plan to later include both them as we might have to redo the same work twice. 
But if you all agree on this plan I'll stick to it.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[cloud-fan]

Github user cloud-fan commented on the issue:

https://github.com/apache/spark/pull/21568
  
It's good to cover both of the cases in one design, but I'd like to 
prioritize the join one.

I feel it's common to try with different optimization/runtime configs and 
make sure we get corrected result. It's more important than the decimal one 
that just saves some typing.

Seems it's hard to reach a consensus of a good design to cover both of the 
cases, how about we just do the join one? i.e. a SQL test file can specify a 
config matrix(we need to design a syntax for it), and the test framework should 
run this test file with specified configs and their values to make sure the 
results all match the golden file.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[mgaido91]

Github user mgaido91 commented on the issue:

https://github.com/apache/spark/pull/21568
  
The main other case here was to improve also the coverage for the join 
operations,  because with the default values we are testing only the broadcast 
join.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[rxin]

Github user rxin commented on the issue:

https://github.com/apache/spark/pull/21568
  
What are the use cases other than decimal? I am not sure if we need to 
build a lot of infrastructure just for one or two use cases.



---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[mgaido91]

Github user mgaido91 commented on the issue:

https://github.com/apache/spark/pull/21568
  
The goal here was to address both cases. The need came out in previous PRs 
in order to avoid to copy and paste the same queries in order to test them with 
different configs. So the idea here was to have an infra which covers both 
cases in order to avoid this copy-and paste (see decimalOperations.sql for 
instance).


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[rxin]

Github user rxin commented on the issue:

https://github.com/apache/spark/pull/21568
  
If they produce different results why do you need any infrastructure for 
them? They are just part of the normal test flow.

If they produce the same result, and you don't want to define the same test 
queries twice, we can create an infra for that. I thought that's what this is 
about?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[mgaido91]

Github user mgaido91 commented on the issue:

https://github.com/apache/spark/pull/21568
  
Sorry,what do you mean by a config matrix? And how would we discriminate 
whether each config should produce the same result or they should produce 
different ones?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[rxin]

Github user rxin commented on the issue:

https://github.com/apache/spark/pull/21568
  
Can you just define a config matrix in the beginning of the file, and each 
file is run with the config matrix?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[mgaido91]

Github user mgaido91 commented on the issue:

https://github.com/apache/spark/pull/21568
  
@rxin then we can do what @maropu suggested, i.e. adding a numeric suffix 
and maybe logging the used configs so even though we.don't have them in the 
test name we can anyway know which of them is failing. What do you think? Do 
you have a better proposal?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[rxin]

Github user rxin commented on the issue:

https://github.com/apache/spark/pull/21568
  
I think it's super confusing to have the config names encoded in file 
names. Makes the names super long and difficult to read, and also hard to 
verify what was set, and difficult to get multiple configs.



---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[mgaido91]

Github user mgaido91 commented on the issue:

https://github.com/apache/spark/pull/21568
  
kindly ping @cloud-fan 


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[mgaido91]

Github user mgaido91 commented on the issue:

https://github.com/apache/spark/pull/21568
  
@maropu I think we can, instead. Since the configs are put as suffix in the 
test case name (this happens also in the same result case) you know which 
configs failed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[maropu]

Github user maropu commented on the issue:

https://github.com/apache/spark/pull/21568
  
In the same result case, I'm worried that we cannot easily understand which 
SQL configs cause failures? 
IMHO `withSQLConf` has the same issue, too;
```
Seq(true, false).foreach { flag =>
  withSQLConf("spark.sql.anyFlag" -> flag.toString) {
...
  }
}
```
In this test case (common patterns?), we cannot understand which case (true 
or false) causes the failure at first glance. For example, can we use 
`withClue` to solve this?

https://github.com/apache/spark/compare/master...maropu:AddConfigInfoInException


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[mgaido91]

Github user mgaido91 commented on the issue:

https://github.com/apache/spark/pull/21568
  
Sorry, @maropu, I didn't get want you mean by

> If we accepted the same output files for that case

may you please explain me?

Anyway, the problem with that proposal is not really about the filenames, 
but it is about adding the configs inside the files as comments. Because if we 
have the same output file we cannot include them in the header...


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[maropu]

Github user maropu commented on the issue:

https://github.com/apache/spark/pull/21568
  
We need to strictly handle the second case, too? If we accepted the same 
output files for that case, we could have simpler output file name rules as I 
described 
[here](https://github.com/apache/spark/pull/21568#discussion_r195892983)?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[mgaido91]

Github user mgaido91 commented on the issue:

https://github.com/apache/spark/pull/21568
  
@rxin the PR does what was suggested in these 2 comments 
https://github.com/apache/spark/pull/20023#issuecomment-358306751 and 
https://github.com/apache/spark/pull/21529#issuecomment-396413144. Basically we 
want to run the same SQL test file with different configs.

We have two cases:
 - Running with different configs produces different output (so different 
golden files);
 - Running with different configs produces the same output (so we have only 
one golden file) but the tests are run against different configs.

The goals are to avoid to copy and paste the same queries after setting 
different configurations (as it was done in `decimalArithmeticOperations`) and 
to be able to improve test coverage for the joins (because with default configs 
we basically always execute broadcast joins).


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[maropu]

Github user maropu commented on the issue:

https://github.com/apache/spark/pull/21568
  
IIUC this pr modified code to run a single test file multiple times in 
`SQLQueryTestSuite` with different configurations.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[rxin]

Github user rxin commented on the issue:

https://github.com/apache/spark/pull/21568
  
I'm confused by the description. What does this PR actually do?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[AmplabJenkins]

Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/21568
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/91859/
Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[AmplabJenkins]

Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/21568
  
Merged build finished. Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[SparkQA]

Github user SparkQA commented on the issue:

https://github.com/apache/spark/pull/21568
  
**[Test build #91859 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91859/testReport)**
 for PR 21568 at commit 
[`ed01ff0`](https://github.com/apache/spark/commit/ed01ff0d40fbe65b3a239b196a90013119ad3580).
 * This patch passes all tests.
 * This patch merges cleanly.
 * This patch adds no public classes.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[AmplabJenkins]

Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/21568
  
Merged build finished. Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[AmplabJenkins]

Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/21568
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 

https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/4040/
Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[AmplabJenkins]

Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/21568
  
Merged build finished. Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[AmplabJenkins]

Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/21568
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 

https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/149/
Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[SparkQA]

Github user SparkQA commented on the issue:

https://github.com/apache/spark/pull/21568
  
**[Test build #91859 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91859/testReport)**
 for PR 21568 at commit 
[`ed01ff0`](https://github.com/apache/spark/commit/ed01ff0d40fbe65b3a239b196a90013119ad3580).


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #21568: [SPARK-24562][TESTS] Support different configs for same ...

[mgaido91]

Github user mgaido91 commented on the issue:

https://github.com/apache/spark/pull/21568
  
cc @cloud-fan 


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org