You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by BryanCutler <gi...@git.apache.org> on 2017/08/31 18:26:11 UTC
[GitHub] spark pull request #19098: [SPARK-21583][HOTFIX] Removed intercept in test c...
GitHub user BryanCutler opened a pull request:
https://github.com/apache/spark/pull/19098
[SPARK-21583][HOTFIX] Removed intercept in test causing failures
Removing a check in the ColumnarBatchSuite that depended on a Java assertion. This assertion is being compiled out in the Maven builds causing the test to fail. This part of the test is not specifically from to the functionality that is being tested here.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/BryanCutler/spark hotfix-ColumnarBatchSuite-assertion
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19098.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #19098
----
commit 567487d6089400527a1b30aca054ea517174a08d
Author: Bryan Cutler <cu...@gmail.com>
Date: 2017-08-31T18:21:06Z
this itercept relies on a Java assertion that could be compiled out, failing the test
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19098: [SPARK-21583][HOTFIX] Removed intercept in test causing ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19098
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19098: [SPARK-21583][HOTFIX] Removed intercept in test causing ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/19098
Hm, I wonder how that results in an assertion? that's a normal error case and shouldn't cause an assert. Is it from a third-party library in this case, like Arrow? really we should fix that somehow so that the user-visible contract for this behavior never involves AssertionError.
Still, assertions ought to be _enabled_ during tests anyway, so I don't see how this doesn't actually fire. If it only affects the Maven build I'd suspect that maybe the scalatest-maven-plugin somehow doesn't turn on assertions? but it has `-ea` in its command line.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19098: [SPARK-21583][HOTFIX] Removed intercept in test causing ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19098
**[Test build #81293 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81293/testReport)** for PR 19098 at commit [`567487d`](https://github.com/apache/spark/commit/567487d6089400527a1b30aca054ea517174a08d).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19098: [SPARK-21583][HOTFIX] Removed intercept in test causing ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19098
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81293/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19098: [SPARK-21583][HOTFIX] Removed intercept in test causing ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/19098
The asserts are wrong if this can be called from user code. It should be `require`. The reason is, basically, exactly this. If you don't have assertions on this argument is accepted.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19098: [SPARK-21583][HOTFIX] Removed intercept in test causing ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19098
LGTM. Merging to master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19098: [SPARK-21583][HOTFIX] Removed intercept in test c...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19098
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19098: [SPARK-21583][HOTFIX] Removed intercept in test c...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19098#discussion_r136413800
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala ---
@@ -1308,10 +1308,6 @@ class ColumnarBatchSuite extends SparkFunSuite {
}
}
- intercept[java.lang.AssertionError] {
- batch.getRow(100)
--- End diff --
Please add another test in a follow-up PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19098: [SPARK-21583][HOTFIX] Removed intercept in test causing ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19098
**[Test build #81293 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81293/testReport)** for PR 19098 at commit [`567487d`](https://github.com/apache/spark/commit/567487d6089400527a1b30aca054ea517174a08d).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19098: [SPARK-21583][HOTFIX] Removed intercept in test c...
Posted by BryanCutler <gi...@git.apache.org>.
Github user BryanCutler commented on a diff in the pull request:
https://github.com/apache/spark/pull/19098#discussion_r136427951
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala ---
@@ -1308,10 +1308,6 @@ class ColumnarBatchSuite extends SparkFunSuite {
}
}
- intercept[java.lang.AssertionError] {
- batch.getRow(100)
--- End diff --
Thanks @gatorsmile , I'll put this in another test once I figure out why it wasn't being hit
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19098: [SPARK-21583][HOTFIX] Removed intercept in test causing ...
Posted by BryanCutler <gi...@git.apache.org>.
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/19098
@srowen , the assertion is from the Spark `ColumnarBatch` [here](https://github.com/apache/spark/blob/master/sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnarBatch.java#L491).
```java
public ColumnarBatch.Row getRow(int rowId) {
assert(rowId >= 0);
assert(rowId < numRows);
row.rowId = rowId;
return row;
}
```
I'm also not quite sure why this wasn't working, I checked and `-ea` is an added argument in the pom. Still, I wonder if we should change the asserts in this class to something better. Maybe these are used instead of exceptions for performance?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org