Skip to content

Migrate Procedure sub-classes in spark-extensions to JUnit5 and AssertJ style #9760

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.

Sign up for GitHub

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

Merged
nastra merged 6 commits into from
Feb 24, 2024
Merged

Migrate Procedure sub-classes in spark-extensions to JUnit5 and AssertJ style #9760

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2d41bef
Migrate Procedure sub-classes in spark-extensions to JUnit5 and Asser…
tomtongue Feb 20, 2024
5f20957
Update the remaining procedures with JUnit5 and AssertJ
tomtongue Feb 21, 2024
8f593a8
Reflect the comments
tomtongue Feb 22, 2024
a375693
Update temp directory creation with Files
tomtongue Feb 23, 2024
e94bd2f
Reflect the comments related to createTempDirectory
tomtongue Feb 23, 2024
ae3d3e6
Update createTempDirectory with specifying the full path
tomtongue Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 12 additions & 15 deletions ...xtensions/src/test/java/org/apache/iceberg/spark/extensions/TestAncestorsOfProcedure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,27 +19,24 @@
package org.apache.iceberg.spark.extensions;

import java.util.List;
import java.util.Map;
import org.apache.iceberg.ParameterizedTestExtension;
import org.apache.iceberg.Table;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.spark.sql.AnalysisException;
import org.assertj.core.api.Assertions;
import org.junit.After;
import org.junit.Test;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.TestTemplate;
import org.junit.jupiter.api.extension.ExtendWith;

public class TestAncestorsOfProcedure extends SparkExtensionsTestBase {
@ExtendWith(ParameterizedTestExtension.class)
public class TestAncestorsOfProcedure extends ExtensionsTestBase {

public TestAncestorsOfProcedure(
String catalogName, String implementation, Map<String, String> config) {
super(catalogName, implementation, config);
}

@After
@AfterEach
public void removeTables() {
sql("DROP TABLE IF EXISTS %s", tableName);
}

@Test
@TestTemplate
public void testAncestorOfUsingEmptyArgs() {
sql("CREATE TABLE %s (id bigint NOT NULL, data string) USING iceberg", tableName);
sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
Expand All @@ -60,7 +57,7 @@ public void testAncestorOfUsingEmptyArgs() {
output);
}

@Test
@TestTemplate
public void testAncestorOfUsingSnapshotId() {
sql("CREATE TABLE %s (id bigint NOT NULL, data string) USING iceberg", tableName);
sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
Expand All @@ -84,7 +81,7 @@ public void testAncestorOfUsingSnapshotId() {
sql("CALL %s.system.ancestors_of('%s', %dL)", catalogName, tableIdent, preSnapshotId));
}

@Test
@TestTemplate
public void testAncestorOfWithRollBack() {
sql("CREATE TABLE %s (id bigint NOT NULL, data string) USING iceberg", tableName);
Table table = validationCatalog.loadTable(tableIdent);
Expand Down Expand Up @@ -128,7 +125,7 @@ public void testAncestorOfWithRollBack() {
sql("CALL %s.system.ancestors_of('%s', %dL)", catalogName, tableIdent, thirdSnapshotId));
}

@Test
@TestTemplate
public void testAncestorOfUsingNamedArgs() {
sql("CREATE TABLE %s (id bigint NOT NULL, data string) USING iceberg", tableName);
sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
Expand All @@ -145,7 +142,7 @@ public void testAncestorOfUsingNamedArgs() {
catalogName, firstSnapshotId, tableIdent));
}

@Test
@TestTemplate
public void testInvalidAncestorOfCases() {
Assertions.assertThatThrownBy(() -> sql("CALL %s.system.ancestors_of()", catalogName))
.isInstanceOf(AnalysisException.class)
Expand Down
65 changes: 31 additions & 34 deletions ...extensions/src/test/java/org/apache/iceberg/spark/extensions/TestCallStatementParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
*/
package org.apache.iceberg.spark.extensions;

import static org.assertj.core.api.Assertions.assertThat;
import static scala.collection.JavaConverters.seqAsJavaList;

import java.math.BigDecimal;
import java.sql.Timestamp;
import java.time.Instant;
Expand All @@ -38,22 +41,16 @@
import org.apache.spark.sql.types.DataType;
import org.apache.spark.sql.types.DataTypes;
import org.assertj.core.api.Assertions;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import scala.collection.JavaConverters;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;

public class TestCallStatementParser {

@Rule public TemporaryFolder temp = new TemporaryFolder();

private static SparkSession spark = null;
private static ParserInterface parser = null;

@BeforeClass
@BeforeAll
public static void startSpark() {
TestCallStatementParser.spark =
SparkSession.builder()
Expand All @@ -64,7 +61,7 @@ public static void startSpark() {
TestCallStatementParser.parser = spark.sessionState().sqlParser();
}

@AfterClass
@AfterAll
public static void stopSpark() {
SparkSession currentSpark = TestCallStatementParser.spark;
TestCallStatementParser.spark = null;
Expand All @@ -76,10 +73,9 @@ public static void stopSpark() {
public void testCallWithPositionalArgs() throws ParseException {
CallStatement call =
(CallStatement) parser.parsePlan("CALL c.n.func(1, '2', 3L, true, 1.0D, 9.0e1, 900e-1BD)");
Assert.assertEquals(
ImmutableList.of("c", "n", "func"), JavaConverters.seqAsJavaList(call.name()));
assertThat(seqAsJavaList(call.name())).hasSameElementsAs(ImmutableList.of("c", "n", "func"));

@nastra nastra Feb 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertThat(seqAsJavaList(call.name())).hasSameElementsAs(ImmutableList.of("c", "n", "func"));
assertThat(seqAsJavaList(call.name())).containsExactly("c", "n", "func");

please also update the other places


Assert.assertEquals(7, call.args().size());
assertThat(seqAsJavaList(call.args())).hasSize(7);

checkArg(call, 0, 1, DataTypes.IntegerType);
checkArg(call, 1, "2", DataTypes.StringType);
Expand All @@ -94,10 +90,10 @@ public void testCallWithPositionalArgs() throws ParseException {
public void testCallWithNamedArgs() throws ParseException {
CallStatement call =
(CallStatement) parser.parsePlan("CALL cat.system.func(c1 => 1, c2 => '2', c3 => true)");
Assert.assertEquals(
ImmutableList.of("cat", "system", "func"), JavaConverters.seqAsJavaList(call.name()));
assertThat(seqAsJavaList(call.name()))
.hasSameElementsAs(ImmutableList.of("cat", "system", "func"));

Assert.assertEquals(3, call.args().size());
assertThat(seqAsJavaList(call.args())).hasSize(3);

checkArg(call, 0, "c1", 1, DataTypes.IntegerType);
checkArg(call, 1, "c2", "2", DataTypes.StringType);
Expand All @@ -107,10 +103,10 @@ public void testCallWithNamedArgs() throws ParseException {
@Test
public void testCallWithMixedArgs() throws ParseException {
CallStatement call = (CallStatement) parser.parsePlan("CALL cat.system.func(c1 => 1, '2')");
Assert.assertEquals(
ImmutableList.of("cat", "system", "func"), JavaConverters.seqAsJavaList(call.name()));
assertThat(seqAsJavaList(call.name()))
.hasSameElementsAs(ImmutableList.of("cat", "system", "func"));

Assert.assertEquals(2, call.args().size());
assertThat(seqAsJavaList(call.args())).hasSize(2);

checkArg(call, 0, "c1", 1, DataTypes.IntegerType);
checkArg(call, 1, "2", DataTypes.StringType);
Expand All @@ -121,10 +117,10 @@ public void testCallWithTimestampArg() throws ParseException {
CallStatement call =
(CallStatement)
parser.parsePlan("CALL cat.system.func(TIMESTAMP '2017-02-03T10:37:30.00Z')");
Assert.assertEquals(
ImmutableList.of("cat", "system", "func"), JavaConverters.seqAsJavaList(call.name()));
assertThat(seqAsJavaList(call.name()))
.hasSameElementsAs(ImmutableList.of("cat", "system", "func"));

Assert.assertEquals(1, call.args().size());
assertThat(seqAsJavaList(call.args())).hasSize(1);

checkArg(
call, 0, Timestamp.from(Instant.parse("2017-02-03T10:37:30.00Z")), DataTypes.TimestampType);
Expand All @@ -134,10 +130,10 @@ public void testCallWithTimestampArg() throws ParseException {
public void testCallWithVarSubstitution() throws ParseException {
CallStatement call =
(CallStatement) parser.parsePlan("CALL cat.system.func('${spark.extra.prop}')");
Assert.assertEquals(
ImmutableList.of("cat", "system", "func"), JavaConverters.seqAsJavaList(call.name()));
assertThat(seqAsJavaList(call.name()))
.hasSameElementsAs(ImmutableList.of("cat", "system", "func"));

Assert.assertEquals(1, call.args().size());
assertThat(seqAsJavaList(call.args())).hasSize(1);

checkArg(call, 0, "value", DataTypes.StringType);
}
Expand Down Expand Up @@ -165,10 +161,10 @@ public void testCallStripsComments() throws ParseException {
"CALL -- a line ending comment\n" + "cat.system.func('${spark.extra.prop}')");
for (String sqlText : callStatementsWithComments) {
CallStatement call = (CallStatement) parser.parsePlan(sqlText);
Assert.assertEquals(
ImmutableList.of("cat", "system", "func"), JavaConverters.seqAsJavaList(call.name()));
assertThat(seqAsJavaList(call.name()))
.hasSameElementsAs(ImmutableList.of("cat", "system", "func"));

Assert.assertEquals(1, call.args().size());
assertThat(seqAsJavaList(call.args())).hasSize(1);

checkArg(call, 0, "value", DataTypes.StringType);
}
Expand All @@ -188,25 +184,26 @@ private void checkArg(

if (expectedName != null) {
NamedArgument arg = checkCast(call.args().apply(index), NamedArgument.class);
Assert.assertEquals(expectedName, arg.name());
assertThat(arg.name()).isEqualTo(expectedName);
} else {
CallArgument arg = call.args().apply(index);
checkCast(arg, PositionalArgument.class);
}

Expression expectedExpr = toSparkLiteral(expectedValue, expectedType);
Expression actualExpr = call.args().apply(index).expr();
Assert.assertEquals("Arg types must match", expectedExpr.dataType(), actualExpr.dataType());
Assert.assertEquals("Arg must match", expectedExpr, actualExpr);
assertThat(actualExpr.dataType()).as("Arg types must match").isEqualTo(expectedExpr.dataType());
assertThat(actualExpr).as("Arg must match").isEqualTo(expectedExpr);
}

private Literal toSparkLiteral(Object value, DataType dataType) {
return Literal$.MODULE$.create(value, dataType);
}

private <T> T checkCast(Object value, Class<T> expectedClass) {
Assert.assertTrue(
"Expected instance of " + expectedClass.getName(), expectedClass.isInstance(value));
assertThat(expectedClass.isInstance(value))

@nastra nastra Feb 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertThat(expectedClass.isInstance(value))
assertThat(expectedClass).isInstanceOf(value.getClass())

@tomtongue tomtongue Feb 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this previously, but the tests related to this method failed with the following error:

// Example
java.lang.AssertionError: 
Expecting actual:
  org.apache.spark.sql.catalyst.plans.logical.PositionalArgument
to be an instance of:
  org.apache.spark.sql.catalyst.plans.logical.PositionalArgument
but was instance of:
  java.lang.Class

value.getClass seems not to be resolved the correct class name inside of .isInstanceOf.

This works well when using the current code like expectedClass.isInstance(value)

@nastra nastra Feb 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem with the current assertion is that it only will print true/false if the assertion fails. What we rather want to achieve for assertions is to have enough context to know what actual/expected was exactly. That being said, we want to avoid the usage of isTrue() / isFalse() as much as possible.

What was the assertion that you previously used?

@tomtongue tomtongue Feb 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reply. Yes, I understand the situation and we should avoid using isTrue()/isFalse().

In the current PR, I change it to assertThat(expectedClass.isInstance(value)).isTrue(),
Before submitting the PR, I tested assertThat(expectedClass).isInstanceOf(value.getClass()), but this failed due to the error above. This is the reason assertThat(expectedClass.isInstance(value)).isTrue() is used here.

@nastra nastra Feb 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry my bad, the check should be assertThat(value).isInstanceOf(expectedClass)

@tomtongue tomtongue Feb 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, you're correct. sorry for that. Let me fix it.

.as("Expected instance of " + expectedClass.getName())

@nastra nastra Feb 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.as("Expected instance of " + expectedClass.getName())

.isTrue();
return expectedClass.cast(value);
}
}
Loading