You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rakesh-das08 (via GitHub)" <gi...@apache.org> on 2023/05/28 17:31:51 UTC
[GitHub] [iceberg] rakesh-das08 opened a new pull request, #7730: Api: Switch iceberg-api tests to Junit5
rakesh-das08 opened a new pull request, #7730:
URL: https://github.com/apache/iceberg/pull/7730
@nastra as suggested by you in #7666 , I would be closing that PR and would raise new PRs which are more review friendly.
This is the first PR for that. In total there are 60 files within the api tests and I would divide them between 3 PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1210445093
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -654,64 +710,73 @@ public void testInExceptions() {
@Test
public void testNotIn() {
- Assert.assertEquals(3, notIn("s", 7, 8, 9).literals().size());
- Assert.assertEquals(3, notIn("s", 7, 8.1, Long.MAX_VALUE).literals().size());
- Assert.assertEquals(3, notIn("s", "abc", "abd", "abc").literals().size());
- Assert.assertEquals(0, notIn("s").literals().size());
- Assert.assertEquals(1, notIn("s", 5).literals().size());
- Assert.assertEquals(2, notIn("s", 5, 5).literals().size());
- Assert.assertEquals(2, notIn("s", Arrays.asList(5, 5)).literals().size());
- Assert.assertEquals(0, notIn("s", Collections.emptyList()).literals().size());
+ assertThat(notIn("s", 7, 8, 9).literals().size()).isEqualTo(3);
Review Comment:
Made the changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1210443701
##########
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java:
##########
@@ -160,87 +158,81 @@ public void testLongToIntegerConversion() {
StructType struct = StructType.of(required(17, "i", Types.IntegerType.get()));
UnboundPredicate<Long> lt = new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than above max should be alwaysTrue", Expressions.alwaysTrue(), lt.bind(struct));
+ assertThat(lt.bind(struct))
+ .as("Less than above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> lteq =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than or equal above max should be alwaysTrue",
- Expressions.alwaysTrue(),
- lteq.bind(struct));
+ assertThat(lteq.bind(struct))
+ .as("Less than or equal above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gt = new UnboundPredicate<>(GT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than below min should be alwaysTrue", Expressions.alwaysTrue(), gt.bind(struct));
+ assertThat(gt.bind(struct))
+ .as("Greater than below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gteq =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than or equal below min should be alwaysTrue",
- Expressions.alwaysTrue(),
- gteq.bind(struct));
+ assertThat(gteq.bind(struct))
+ .as("Greater than or equal below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gtMax =
new UnboundPredicate<>(GT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gtMax.bind(struct));
+ assertThat(gtMax.bind(struct))
+ .as("Greater than above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> gteqMax =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than or equal above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gteqMax.bind(struct));
+ assertThat(gteqMax.bind(struct))
+ .as("Greater than or equal above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> ltMin =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than below min should be alwaysFalse", Expressions.alwaysFalse(), ltMin.bind(struct));
+ assertThat(ltMin.bind(struct))
+ .as("Less than below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> lteqMin =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than or equal below min should be alwaysFalse",
- Expressions.alwaysFalse(),
- lteqMin.bind(struct));
+ assertThat(lteqMin.bind(struct))
+ .as("Less than or equal below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
Expression ltExpr =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE).bind(struct, true);
BoundPredicate<Integer> ltMax = assertAndUnwrap(ltExpr);
- Assert.assertTrue("Should be a literal predicate", ltMax.isLiteralPredicate());
- Assert.assertEquals(
- "Should translate bound to Integer",
- (Integer) Integer.MAX_VALUE,
- ltMax.asLiteralPredicate().literal().value());
+ assertThat(ltMax.isLiteralPredicate()).as("Should be a literal predicate").isTrue();
+ assertThat(ltMax.asLiteralPredicate().literal().value())
+ .as("Should translate bound to Integer")
+ .isEqualTo((Integer) Integer.MAX_VALUE);
Expression lteqExpr =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MAX_VALUE).bind(struct);
BoundPredicate<Integer> lteqMax = assertAndUnwrap(lteqExpr);
- Assert.assertTrue("Should be a literal predicate", lteqMax.isLiteralPredicate());
- Assert.assertEquals(
- "Should translate bound to Integer",
- (Integer) Integer.MAX_VALUE,
- lteqMax.asLiteralPredicate().literal().value());
+ assertThat(lteqMax.isLiteralPredicate()).as("Should be a literal predicate").isTrue();
+ assertThat(lteqMax.asLiteralPredicate().literal().value())
+ .as("Should translate bound to Integer")
+ .isEqualTo((Integer) Integer.MAX_VALUE);
Review Comment:
Removed these unnecessary typecasts from assertions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1213960711
##########
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java:
##########
@@ -134,7 +134,7 @@ public void testLiteralConversion() {
assertThat(bound.isLiteralPredicate()).isTrue();
assertThat(bound.asLiteralPredicate().literal().value())
.as("Should convert literal value to decimal")
- .isEqualTo(new BigDecimal("12.40"));
+ .isEqualTo("12.40");
Review Comment:
we can leave the BigDecimal stuff because `bound.asLiteralPredicate().literal().value()` will produce a `BigDecimal` and so we're expecting a `BigDecimal` here. The only reason this test passes is actually because `isEqualTo()` will convert that String to a `BigDecimal` again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7730: API: Switch tests to JUnit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1215447617
##########
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java:
##########
@@ -66,13 +66,12 @@ public void testMultipleFields() {
Expression expr = unbound.bind(struct);
BoundPredicate<Integer> bound = assertAndUnwrap(expr);
- Assert.assertEquals("Should reference correct field ID", 11, bound.ref().fieldId());
- Assert.assertEquals("Should not change the comparison operation", LT, bound.op());
- Assert.assertTrue("Should be a literal predicate", bound.isLiteralPredicate());
- Assert.assertEquals(
- "Should not alter literal value",
- Integer.valueOf(6),
- bound.asLiteralPredicate().literal().value());
+ assertThat(bound.ref().fieldId()).as("Should reference correct field ID").isEqualTo(11);
+ assertThat(bound.op()).as("Should not change the comparison operation").isEqualTo(LT);
+ assertThat(bound.isLiteralPredicate()).isTrue();
+ assertThat(bound.asLiteralPredicate().literal().value())
+ .as("Should not alter literal value")
+ .isEqualTo(Integer.valueOf(6));
Review Comment:
```suggestion
.isEqualTo(6);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1211787011
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -159,12 +148,9 @@ public void testDoubleToDecimalConversion() {
assertThat(lit.to(Types.DecimalType.of(9, 1)).value())
.as("Value should round using HALF_UP")
.isEqualTo(new BigDecimal("34.6"));
- assertThat(lit.to(Types.DecimalType.of(9, 2)).value())
- .as("Value should match")
- .isEqualTo(new BigDecimal("34.56"));
- assertThat(lit.to(Types.DecimalType.of(9, 4)).value())
- .as("Value should match")
- .isEqualTo(new BigDecimal("34.5600"));
+ assertThat(lit.to(Types.DecimalType.of(9, 2)).value()).isEqualTo(new BigDecimal("34.56"));
+
Review Comment:
removed the new line
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -127,20 +119,17 @@ public void testFloatToDecimalConversion() {
assertThat(lit.to(Types.DecimalType.of(9, 1)).value())
.as("Value should round using HALF_UP")
.isEqualTo(new BigDecimal("34.6"));
- assertThat(lit.to(Types.DecimalType.of(9, 2)).value())
- .as("Value should match")
- .isEqualTo(new BigDecimal("34.56"));
- assertThat(lit.to(Types.DecimalType.of(9, 4)).value())
- .as("Value should match")
- .isEqualTo(new BigDecimal("34.5600"));
+ assertThat(lit.to(Types.DecimalType.of(9, 2)).value()).isEqualTo(new BigDecimal("34.56"));
+
Review Comment:
removed the new line
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -18,155 +18,139 @@
*/
package org.apache.iceberg.expressions;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.data.Offset.offset;
+
import java.math.BigDecimal;
import java.util.stream.IntStream;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestNumericLiteralConversions {
@Test
public void testIntegerToLongConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Long> longLit = lit.to(Types.LongType.get());
- Assert.assertEquals("Value should match", 34L, (long) longLit.value());
+ assertThat((long) longLit.value()).isEqualTo(34L);
}
@Test
public void testIntegerToFloatConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Float> floatLit = lit.to(Types.FloatType.get());
- Assert.assertEquals("Value should match", 34.0F, floatLit.value(), 0.0000000001D);
+ assertThat(floatLit.value()).isCloseTo(34.0F, offset(0.0000000001F));
}
@Test
public void testIntegerToDoubleConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Double> doubleLit = lit.to(Types.DoubleType.get());
- Assert.assertEquals("Value should match", 34.0D, doubleLit.value(), 0.0000000001D);
+ assertThat(doubleLit.value()).isCloseTo(34.0D, offset(0.0000000001D));
}
@Test
public void testIntegerToDecimalConversion() {
Literal<Integer> lit = Literal.of(34);
- Assert.assertEquals(
- "Value should match", new BigDecimal("34"), lit.to(Types.DecimalType.of(9, 0)).value());
- Assert.assertEquals(
- "Value should match", new BigDecimal("34.00"), lit.to(Types.DecimalType.of(9, 2)).value());
- Assert.assertEquals(
- "Value should match",
- new BigDecimal("34.0000"),
- lit.to(Types.DecimalType.of(9, 4)).value());
+ assertThat(lit.to(Types.DecimalType.of(9, 0)).value()).isEqualTo(new BigDecimal("34"));
+
Review Comment:
removed the new line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#issuecomment-1573823691
Thank you for the time you have spent on this review. Will take care of the comments made by you in this PR while raising the next 2 PRs for API module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1210444608
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -552,72 +599,81 @@ public void testCaseSensitiveNot() {
public void testCharSeqValue() {
StructType struct = StructType.of(required(34, "s", Types.StringType.get()));
Evaluator evaluator = new Evaluator(struct, equal("s", "abc"));
- Assert.assertTrue(
- "string(abc) == utf8(abc) => true", evaluator.eval(TestHelpers.Row.of(new Utf8("abc"))));
- Assert.assertFalse(
- "string(abc) == utf8(abcd) => false", evaluator.eval(TestHelpers.Row.of(new Utf8("abcd"))));
+ assertThat(evaluator.eval(TestHelpers.Row.of(new Utf8("abc"))))
+ .as("string(abc) == utf8(abc) => true")
+ .isTrue();
+ assertThat(evaluator.eval(TestHelpers.Row.of(new Utf8("abcd"))))
+ .as("string(abc) == utf8(abcd) => false")
+ .isFalse();
}
@Test
public void testIn() {
- Assert.assertEquals(3, in("s", 7, 8, 9).literals().size());
- Assert.assertEquals(3, in("s", 7, 8.1, Long.MAX_VALUE).literals().size());
- Assert.assertEquals(3, in("s", "abc", "abd", "abc").literals().size());
- Assert.assertEquals(0, in("s").literals().size());
- Assert.assertEquals(1, in("s", 5).literals().size());
- Assert.assertEquals(2, in("s", 5, 5).literals().size());
- Assert.assertEquals(2, in("s", Arrays.asList(5, 5)).literals().size());
- Assert.assertEquals(0, in("s", Collections.emptyList()).literals().size());
+ assertThat(in("s", 7, 8, 9).literals().size()).isEqualTo(3);
Review Comment:
Somehow missed this file for the suggested changes. Thank you. Have made the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1211857090
##########
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java:
##########
@@ -160,87 +158,81 @@ public void testLongToIntegerConversion() {
StructType struct = StructType.of(required(17, "i", Types.IntegerType.get()));
UnboundPredicate<Long> lt = new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than above max should be alwaysTrue", Expressions.alwaysTrue(), lt.bind(struct));
+ assertThat(lt.bind(struct))
+ .as("Less than above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> lteq =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than or equal above max should be alwaysTrue",
- Expressions.alwaysTrue(),
- lteq.bind(struct));
+ assertThat(lteq.bind(struct))
+ .as("Less than or equal above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gt = new UnboundPredicate<>(GT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than below min should be alwaysTrue", Expressions.alwaysTrue(), gt.bind(struct));
+ assertThat(gt.bind(struct))
+ .as("Greater than below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gteq =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than or equal below min should be alwaysTrue",
- Expressions.alwaysTrue(),
- gteq.bind(struct));
+ assertThat(gteq.bind(struct))
+ .as("Greater than or equal below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gtMax =
new UnboundPredicate<>(GT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gtMax.bind(struct));
+ assertThat(gtMax.bind(struct))
+ .as("Greater than above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> gteqMax =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than or equal above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gteqMax.bind(struct));
+ assertThat(gteqMax.bind(struct))
+ .as("Greater than or equal above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> ltMin =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than below min should be alwaysFalse", Expressions.alwaysFalse(), ltMin.bind(struct));
+ assertThat(ltMin.bind(struct))
+ .as("Less than below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> lteqMin =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than or equal below min should be alwaysFalse",
- Expressions.alwaysFalse(),
- lteqMin.bind(struct));
+ assertThat(lteqMin.bind(struct))
+ .as("Less than or equal below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
Expression ltExpr =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE).bind(struct, true);
BoundPredicate<Integer> ltMax = assertAndUnwrap(ltExpr);
- Assert.assertTrue("Should be a literal predicate", ltMax.isLiteralPredicate());
- Assert.assertEquals(
- "Should translate bound to Integer",
- (Integer) Integer.MAX_VALUE,
- ltMax.asLiteralPredicate().literal().value());
+ assertThat(ltMax.isLiteralPredicate()).isTrue();
+ assertThat(ltMax.asLiteralPredicate().literal().value())
+ .as("Should translate bound to Integer")
+ .isEqualTo((Integer) Integer.MAX_VALUE);
Review Comment:
there are still type casts left like this one which we don't really need to do
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1209745291
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -18,155 +18,153 @@
*/
package org.apache.iceberg.expressions;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.data.Offset.offset;
+
import java.math.BigDecimal;
import java.util.stream.IntStream;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestNumericLiteralConversions {
@Test
public void testIntegerToLongConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Long> longLit = lit.to(Types.LongType.get());
- Assert.assertEquals("Value should match", 34L, (long) longLit.value());
+ assertThat((long) longLit.value()).as("Value should match").isEqualTo(34L);
}
@Test
public void testIntegerToFloatConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Float> floatLit = lit.to(Types.FloatType.get());
- Assert.assertEquals("Value should match", 34.0F, floatLit.value(), 0.0000000001D);
+ assertThat(floatLit.value()).as("Value should match").isCloseTo(34.0F, offset(0.0000000001F));
}
@Test
public void testIntegerToDoubleConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Double> doubleLit = lit.to(Types.DoubleType.get());
- Assert.assertEquals("Value should match", 34.0D, doubleLit.value(), 0.0000000001D);
+ assertThat(doubleLit.value()).as("Value should match").isCloseTo(34.0D, offset(0.0000000001D));
}
@Test
public void testIntegerToDecimalConversion() {
Literal<Integer> lit = Literal.of(34);
- Assert.assertEquals(
- "Value should match", new BigDecimal("34"), lit.to(Types.DecimalType.of(9, 0)).value());
- Assert.assertEquals(
- "Value should match", new BigDecimal("34.00"), lit.to(Types.DecimalType.of(9, 2)).value());
- Assert.assertEquals(
- "Value should match",
- new BigDecimal("34.0000"),
- lit.to(Types.DecimalType.of(9, 4)).value());
+ assertThat(lit.to(Types.DecimalType.of(9, 0)).value())
+ .as("Value should match")
Review Comment:
nit: going forward I think we can omit the `.as(..)` for something that's obvious because we always expect the value to match
##########
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java:
##########
@@ -160,87 +158,81 @@ public void testLongToIntegerConversion() {
StructType struct = StructType.of(required(17, "i", Types.IntegerType.get()));
UnboundPredicate<Long> lt = new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than above max should be alwaysTrue", Expressions.alwaysTrue(), lt.bind(struct));
+ assertThat(lt.bind(struct))
+ .as("Less than above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> lteq =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than or equal above max should be alwaysTrue",
- Expressions.alwaysTrue(),
- lteq.bind(struct));
+ assertThat(lteq.bind(struct))
+ .as("Less than or equal above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gt = new UnboundPredicate<>(GT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than below min should be alwaysTrue", Expressions.alwaysTrue(), gt.bind(struct));
+ assertThat(gt.bind(struct))
+ .as("Greater than below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gteq =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than or equal below min should be alwaysTrue",
- Expressions.alwaysTrue(),
- gteq.bind(struct));
+ assertThat(gteq.bind(struct))
+ .as("Greater than or equal below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gtMax =
new UnboundPredicate<>(GT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gtMax.bind(struct));
+ assertThat(gtMax.bind(struct))
+ .as("Greater than above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> gteqMax =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than or equal above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gteqMax.bind(struct));
+ assertThat(gteqMax.bind(struct))
+ .as("Greater than or equal above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> ltMin =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than below min should be alwaysFalse", Expressions.alwaysFalse(), ltMin.bind(struct));
+ assertThat(ltMin.bind(struct))
+ .as("Less than below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> lteqMin =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than or equal below min should be alwaysFalse",
- Expressions.alwaysFalse(),
- lteqMin.bind(struct));
+ assertThat(lteqMin.bind(struct))
+ .as("Less than or equal below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
Expression ltExpr =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE).bind(struct, true);
BoundPredicate<Integer> ltMax = assertAndUnwrap(ltExpr);
- Assert.assertTrue("Should be a literal predicate", ltMax.isLiteralPredicate());
- Assert.assertEquals(
- "Should translate bound to Integer",
- (Integer) Integer.MAX_VALUE,
- ltMax.asLiteralPredicate().literal().value());
+ assertThat(ltMax.isLiteralPredicate()).as("Should be a literal predicate").isTrue();
+ assertThat(ltMax.asLiteralPredicate().literal().value())
+ .as("Should translate bound to Integer")
+ .isEqualTo((Integer) Integer.MAX_VALUE);
Expression lteqExpr =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MAX_VALUE).bind(struct);
BoundPredicate<Integer> lteqMax = assertAndUnwrap(lteqExpr);
- Assert.assertTrue("Should be a literal predicate", lteqMax.isLiteralPredicate());
- Assert.assertEquals(
- "Should translate bound to Integer",
- (Integer) Integer.MAX_VALUE,
- lteqMax.asLiteralPredicate().literal().value());
+ assertThat(lteqMax.isLiteralPredicate()).as("Should be a literal predicate").isTrue();
+ assertThat(lteqMax.asLiteralPredicate().literal().value())
+ .as("Should translate bound to Integer")
+ .isEqualTo((Integer) Integer.MAX_VALUE);
Review Comment:
I don't think the type casting in all of these assertions within that file are actually required
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -176,30 +174,36 @@ public void testDecimalToDecimalConversion() {
IntStream.range(0, 10)
.forEach(
scale -> {
- Assert.assertSame(
- "Should return identical object", lit, lit.to(Types.DecimalType.of(9, scale)));
- Assert.assertSame(
- "Should return identical object", lit, lit.to(Types.DecimalType.of(11, scale)));
+ assertThat(lit.to(Types.DecimalType.of(9, scale)))
+ .as("Should return identical object")
+ .isSameAs(lit);
+ assertThat(lit.to(Types.DecimalType.of(11, scale)))
+ .as("Should return identical object")
+ .isSameAs(lit);
});
}
@Test
public void testIntegerToDateConversion() {
Literal<Integer> lit = Literal.of(0);
- Assert.assertEquals(
- "Dates should be equal", lit.to(Types.DateType.get()), new Literals.DateLiteral(0));
+ assertThat(new Literals.DateLiteral(0))
+ .as("Dates should be equal")
Review Comment:
same for this kind of `.as()` message. In a equality comparison we always expect two given values to be equal, so going forward I think it's ok to omit this kind of `.as()` msg
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -654,64 +710,73 @@ public void testInExceptions() {
@Test
public void testNotIn() {
- Assert.assertEquals(3, notIn("s", 7, 8, 9).literals().size());
- Assert.assertEquals(3, notIn("s", 7, 8.1, Long.MAX_VALUE).literals().size());
- Assert.assertEquals(3, notIn("s", "abc", "abd", "abc").literals().size());
- Assert.assertEquals(0, notIn("s").literals().size());
- Assert.assertEquals(1, notIn("s", 5).literals().size());
- Assert.assertEquals(2, notIn("s", 5, 5).literals().size());
- Assert.assertEquals(2, notIn("s", Arrays.asList(5, 5)).literals().size());
- Assert.assertEquals(0, notIn("s", Collections.emptyList()).literals().size());
+ assertThat(notIn("s", 7, 8, 9).literals().size()).isEqualTo(3);
Review Comment:
same as above
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -552,72 +599,81 @@ public void testCaseSensitiveNot() {
public void testCharSeqValue() {
StructType struct = StructType.of(required(34, "s", Types.StringType.get()));
Evaluator evaluator = new Evaluator(struct, equal("s", "abc"));
- Assert.assertTrue(
- "string(abc) == utf8(abc) => true", evaluator.eval(TestHelpers.Row.of(new Utf8("abc"))));
- Assert.assertFalse(
- "string(abc) == utf8(abcd) => false", evaluator.eval(TestHelpers.Row.of(new Utf8("abcd"))));
+ assertThat(evaluator.eval(TestHelpers.Row.of(new Utf8("abc"))))
+ .as("string(abc) == utf8(abc) => true")
+ .isTrue();
+ assertThat(evaluator.eval(TestHelpers.Row.of(new Utf8("abcd"))))
+ .as("string(abc) == utf8(abcd) => false")
+ .isFalse();
}
@Test
public void testIn() {
- Assert.assertEquals(3, in("s", 7, 8, 9).literals().size());
- Assert.assertEquals(3, in("s", 7, 8.1, Long.MAX_VALUE).literals().size());
- Assert.assertEquals(3, in("s", "abc", "abd", "abc").literals().size());
- Assert.assertEquals(0, in("s").literals().size());
- Assert.assertEquals(1, in("s", 5).literals().size());
- Assert.assertEquals(2, in("s", 5, 5).literals().size());
- Assert.assertEquals(2, in("s", Arrays.asList(5, 5)).literals().size());
- Assert.assertEquals(0, in("s", Collections.emptyList()).literals().size());
+ assertThat(in("s", 7, 8, 9).literals().size()).isEqualTo(3);
Review Comment:
these all can be switched to `assertThat(in(...).literals()).hasSize(...)` as that will show the content of `literals()` when the check ever fails
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1210443049
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -176,30 +174,36 @@ public void testDecimalToDecimalConversion() {
IntStream.range(0, 10)
.forEach(
scale -> {
- Assert.assertSame(
- "Should return identical object", lit, lit.to(Types.DecimalType.of(9, scale)));
- Assert.assertSame(
- "Should return identical object", lit, lit.to(Types.DecimalType.of(11, scale)));
+ assertThat(lit.to(Types.DecimalType.of(9, scale)))
+ .as("Should return identical object")
+ .isSameAs(lit);
+ assertThat(lit.to(Types.DecimalType.of(11, scale)))
+ .as("Should return identical object")
+ .isSameAs(lit);
});
}
@Test
public void testIntegerToDateConversion() {
Literal<Integer> lit = Literal.of(0);
- Assert.assertEquals(
- "Dates should be equal", lit.to(Types.DateType.get()), new Literals.DateLiteral(0));
+ assertThat(new Literals.DateLiteral(0))
+ .as("Dates should be equal")
Review Comment:
Same as above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra merged pull request #7730: API: Switch tests to JUnit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #7730:
URL: https://github.com/apache/iceberg/pull/7730
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1214445673
##########
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java:
##########
@@ -134,7 +134,7 @@ public void testLiteralConversion() {
assertThat(bound.isLiteralPredicate()).isTrue();
assertThat(bound.asLiteralPredicate().literal().value())
.as("Should convert literal value to decimal")
- .isEqualTo(new BigDecimal("12.40"));
+ .isEqualTo("12.40");
Review Comment:
Thank you for the explanation @nastra
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1211889035
##########
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java:
##########
@@ -160,87 +158,81 @@ public void testLongToIntegerConversion() {
StructType struct = StructType.of(required(17, "i", Types.IntegerType.get()));
UnboundPredicate<Long> lt = new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than above max should be alwaysTrue", Expressions.alwaysTrue(), lt.bind(struct));
+ assertThat(lt.bind(struct))
+ .as("Less than above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> lteq =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than or equal above max should be alwaysTrue",
- Expressions.alwaysTrue(),
- lteq.bind(struct));
+ assertThat(lteq.bind(struct))
+ .as("Less than or equal above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gt = new UnboundPredicate<>(GT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than below min should be alwaysTrue", Expressions.alwaysTrue(), gt.bind(struct));
+ assertThat(gt.bind(struct))
+ .as("Greater than below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gteq =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than or equal below min should be alwaysTrue",
- Expressions.alwaysTrue(),
- gteq.bind(struct));
+ assertThat(gteq.bind(struct))
+ .as("Greater than or equal below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gtMax =
new UnboundPredicate<>(GT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gtMax.bind(struct));
+ assertThat(gtMax.bind(struct))
+ .as("Greater than above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> gteqMax =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than or equal above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gteqMax.bind(struct));
+ assertThat(gteqMax.bind(struct))
+ .as("Greater than or equal above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> ltMin =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than below min should be alwaysFalse", Expressions.alwaysFalse(), ltMin.bind(struct));
+ assertThat(ltMin.bind(struct))
+ .as("Less than below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> lteqMin =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than or equal below min should be alwaysFalse",
- Expressions.alwaysFalse(),
- lteqMin.bind(struct));
+ assertThat(lteqMin.bind(struct))
+ .as("Less than or equal below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
Expression ltExpr =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE).bind(struct, true);
BoundPredicate<Integer> ltMax = assertAndUnwrap(ltExpr);
- Assert.assertTrue("Should be a literal predicate", ltMax.isLiteralPredicate());
- Assert.assertEquals(
- "Should translate bound to Integer",
- (Integer) Integer.MAX_VALUE,
- ltMax.asLiteralPredicate().literal().value());
+ assertThat(ltMax.isLiteralPredicate()).isTrue();
+ assertThat(ltMax.asLiteralPredicate().literal().value())
+ .as("Should translate bound to Integer")
+ .isEqualTo((Integer) Integer.MAX_VALUE);
Review Comment:
sry for this back and forth. found the places where redundant typecasts were there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1211222502
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -127,20 +119,17 @@ public void testFloatToDecimalConversion() {
assertThat(lit.to(Types.DecimalType.of(9, 1)).value())
.as("Value should round using HALF_UP")
.isEqualTo(new BigDecimal("34.6"));
- assertThat(lit.to(Types.DecimalType.of(9, 2)).value())
- .as("Value should match")
- .isEqualTo(new BigDecimal("34.56"));
- assertThat(lit.to(Types.DecimalType.of(9, 4)).value())
- .as("Value should match")
- .isEqualTo(new BigDecimal("34.5600"));
+ assertThat(lit.to(Types.DecimalType.of(9, 2)).value()).isEqualTo(new BigDecimal("34.56"));
+
Review Comment:
nit: can we remove this newline?
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -159,12 +148,9 @@ public void testDoubleToDecimalConversion() {
assertThat(lit.to(Types.DecimalType.of(9, 1)).value())
.as("Value should round using HALF_UP")
.isEqualTo(new BigDecimal("34.6"));
- assertThat(lit.to(Types.DecimalType.of(9, 2)).value())
- .as("Value should match")
- .isEqualTo(new BigDecimal("34.56"));
- assertThat(lit.to(Types.DecimalType.of(9, 4)).value())
- .as("Value should match")
- .isEqualTo(new BigDecimal("34.5600"));
+ assertThat(lit.to(Types.DecimalType.of(9, 2)).value()).isEqualTo(new BigDecimal("34.56"));
+
Review Comment:
nit: can we remove this unnecessary newline?
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -18,155 +18,139 @@
*/
package org.apache.iceberg.expressions;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.data.Offset.offset;
+
import java.math.BigDecimal;
import java.util.stream.IntStream;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestNumericLiteralConversions {
@Test
public void testIntegerToLongConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Long> longLit = lit.to(Types.LongType.get());
- Assert.assertEquals("Value should match", 34L, (long) longLit.value());
+ assertThat((long) longLit.value()).isEqualTo(34L);
}
@Test
public void testIntegerToFloatConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Float> floatLit = lit.to(Types.FloatType.get());
- Assert.assertEquals("Value should match", 34.0F, floatLit.value(), 0.0000000001D);
+ assertThat(floatLit.value()).isCloseTo(34.0F, offset(0.0000000001F));
}
@Test
public void testIntegerToDoubleConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Double> doubleLit = lit.to(Types.DoubleType.get());
- Assert.assertEquals("Value should match", 34.0D, doubleLit.value(), 0.0000000001D);
+ assertThat(doubleLit.value()).isCloseTo(34.0D, offset(0.0000000001D));
}
@Test
public void testIntegerToDecimalConversion() {
Literal<Integer> lit = Literal.of(34);
- Assert.assertEquals(
- "Value should match", new BigDecimal("34"), lit.to(Types.DecimalType.of(9, 0)).value());
- Assert.assertEquals(
- "Value should match", new BigDecimal("34.00"), lit.to(Types.DecimalType.of(9, 2)).value());
- Assert.assertEquals(
- "Value should match",
- new BigDecimal("34.0000"),
- lit.to(Types.DecimalType.of(9, 4)).value());
+ assertThat(lit.to(Types.DecimalType.of(9, 0)).value()).isEqualTo(new BigDecimal("34"));
+
Review Comment:
nit: can we remove these unnecessary newlines?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#issuecomment-1568632662
@nastra Thank you for taking out time and reviewing. I have made changes as per your review comments. Have not marked the conversations as resolved for now. Once you review I can do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1211863574
##########
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java:
##########
@@ -160,87 +158,81 @@ public void testLongToIntegerConversion() {
StructType struct = StructType.of(required(17, "i", Types.IntegerType.get()));
UnboundPredicate<Long> lt = new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than above max should be alwaysTrue", Expressions.alwaysTrue(), lt.bind(struct));
+ assertThat(lt.bind(struct))
+ .as("Less than above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> lteq =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than or equal above max should be alwaysTrue",
- Expressions.alwaysTrue(),
- lteq.bind(struct));
+ assertThat(lteq.bind(struct))
+ .as("Less than or equal above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gt = new UnboundPredicate<>(GT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than below min should be alwaysTrue", Expressions.alwaysTrue(), gt.bind(struct));
+ assertThat(gt.bind(struct))
+ .as("Greater than below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gteq =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than or equal below min should be alwaysTrue",
- Expressions.alwaysTrue(),
- gteq.bind(struct));
+ assertThat(gteq.bind(struct))
+ .as("Greater than or equal below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gtMax =
new UnboundPredicate<>(GT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gtMax.bind(struct));
+ assertThat(gtMax.bind(struct))
+ .as("Greater than above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> gteqMax =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than or equal above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gteqMax.bind(struct));
+ assertThat(gteqMax.bind(struct))
+ .as("Greater than or equal above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> ltMin =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than below min should be alwaysFalse", Expressions.alwaysFalse(), ltMin.bind(struct));
+ assertThat(ltMin.bind(struct))
+ .as("Less than below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> lteqMin =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than or equal below min should be alwaysFalse",
- Expressions.alwaysFalse(),
- lteqMin.bind(struct));
+ assertThat(lteqMin.bind(struct))
+ .as("Less than or equal below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
Expression ltExpr =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE).bind(struct, true);
BoundPredicate<Integer> ltMax = assertAndUnwrap(ltExpr);
- Assert.assertTrue("Should be a literal predicate", ltMax.isLiteralPredicate());
- Assert.assertEquals(
- "Should translate bound to Integer",
- (Integer) Integer.MAX_VALUE,
- ltMax.asLiteralPredicate().literal().value());
+ assertThat(ltMax.isLiteralPredicate()).isTrue();
+ assertThat(ltMax.asLiteralPredicate().literal().value())
+ .as("Should translate bound to Integer")
+ .isEqualTo((Integer) Integer.MAX_VALUE);
Review Comment:
I think there are a few more typecasts that we can remove
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1211860713
##########
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java:
##########
@@ -160,87 +158,81 @@ public void testLongToIntegerConversion() {
StructType struct = StructType.of(required(17, "i", Types.IntegerType.get()));
UnboundPredicate<Long> lt = new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than above max should be alwaysTrue", Expressions.alwaysTrue(), lt.bind(struct));
+ assertThat(lt.bind(struct))
+ .as("Less than above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> lteq =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than or equal above max should be alwaysTrue",
- Expressions.alwaysTrue(),
- lteq.bind(struct));
+ assertThat(lteq.bind(struct))
+ .as("Less than or equal above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gt = new UnboundPredicate<>(GT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than below min should be alwaysTrue", Expressions.alwaysTrue(), gt.bind(struct));
+ assertThat(gt.bind(struct))
+ .as("Greater than below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gteq =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than or equal below min should be alwaysTrue",
- Expressions.alwaysTrue(),
- gteq.bind(struct));
+ assertThat(gteq.bind(struct))
+ .as("Greater than or equal below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gtMax =
new UnboundPredicate<>(GT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gtMax.bind(struct));
+ assertThat(gtMax.bind(struct))
+ .as("Greater than above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> gteqMax =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than or equal above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gteqMax.bind(struct));
+ assertThat(gteqMax.bind(struct))
+ .as("Greater than or equal above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> ltMin =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than below min should be alwaysFalse", Expressions.alwaysFalse(), ltMin.bind(struct));
+ assertThat(ltMin.bind(struct))
+ .as("Less than below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> lteqMin =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than or equal below min should be alwaysFalse",
- Expressions.alwaysFalse(),
- lteqMin.bind(struct));
+ assertThat(lteqMin.bind(struct))
+ .as("Less than or equal below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
Expression ltExpr =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE).bind(struct, true);
BoundPredicate<Integer> ltMax = assertAndUnwrap(ltExpr);
- Assert.assertTrue("Should be a literal predicate", ltMax.isLiteralPredicate());
- Assert.assertEquals(
- "Should translate bound to Integer",
- (Integer) Integer.MAX_VALUE,
- ltMax.asLiteralPredicate().literal().value());
+ assertThat(ltMax.isLiteralPredicate()).isTrue();
+ assertThat(ltMax.asLiteralPredicate().literal().value())
+ .as("Should translate bound to Integer")
+ .isEqualTo((Integer) Integer.MAX_VALUE);
Review Comment:
sry abt missing this. Removed this typecast
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1210442623
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -18,155 +18,153 @@
*/
package org.apache.iceberg.expressions;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.data.Offset.offset;
+
import java.math.BigDecimal;
import java.util.stream.IntStream;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestNumericLiteralConversions {
@Test
public void testIntegerToLongConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Long> longLit = lit.to(Types.LongType.get());
- Assert.assertEquals("Value should match", 34L, (long) longLit.value());
+ assertThat((long) longLit.value()).as("Value should match").isEqualTo(34L);
}
@Test
public void testIntegerToFloatConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Float> floatLit = lit.to(Types.FloatType.get());
- Assert.assertEquals("Value should match", 34.0F, floatLit.value(), 0.0000000001D);
+ assertThat(floatLit.value()).as("Value should match").isCloseTo(34.0F, offset(0.0000000001F));
}
@Test
public void testIntegerToDoubleConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Double> doubleLit = lit.to(Types.DoubleType.get());
- Assert.assertEquals("Value should match", 34.0D, doubleLit.value(), 0.0000000001D);
+ assertThat(doubleLit.value()).as("Value should match").isCloseTo(34.0D, offset(0.0000000001D));
}
@Test
public void testIntegerToDecimalConversion() {
Literal<Integer> lit = Literal.of(34);
- Assert.assertEquals(
- "Value should match", new BigDecimal("34"), lit.to(Types.DecimalType.of(9, 0)).value());
- Assert.assertEquals(
- "Value should match", new BigDecimal("34.00"), lit.to(Types.DecimalType.of(9, 2)).value());
- Assert.assertEquals(
- "Value should match",
- new BigDecimal("34.0000"),
- lit.to(Types.DecimalType.of(9, 4)).value());
+ assertThat(lit.to(Types.DecimalType.of(9, 0)).value())
+ .as("Value should match")
Review Comment:
Sure. Made changes wherever i thought obvious description of tests is there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org