[SPARK-21583][HOTFIX] Removed intercept in test causing failures

[BryanCutler]

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.

[gatorsmile]

Please add another test in a follow-up PR.

[BryanCutler]

Thanks @gatorsmile , I'll put this in another test once I figure out why it wasn't being hit

[gatorsmile]

LGTM. Merging to master.

[srowen]

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.

[BryanCutler]

@srowen , the assertion is from the Spark ColumnarBatch here.

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?

[srowen]

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.

[SparkQA]

Test build #81293 has finished for PR 19098 at commit 567487d.

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