slinkydeveloper commented on a change in pull request #17932:
URL: https://github.com/apache/flink/pull/17932#discussion_r759115064
##########
File path:
flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/utils/DataTypeUtilsTest.java
##########
@@ -68,92 +66,79 @@ public void testProjectRow() {
final DataType topLevelRow =
ROW(FIELD("a0", INT()), FIELD("a1", secondLevelRow),
FIELD("a1_b1_c0", INT()));
- assertThat(
- DataTypeUtils.projectRow(topLevelRow, new int[][] {{0}, {1, 1,
0}}),
- equalTo(ROW(FIELD("a0", INT()), FIELD("a1_b1_c0",
BOOLEAN()))));
+ assertThat(DataTypeUtils.projectRow(topLevelRow, new int[][] {{0}, {1,
1, 0}}))
+ .isEqualTo(ROW(FIELD("a0", INT()), FIELD("a1_b1_c0",
BOOLEAN())));
- assertThat(
- DataTypeUtils.projectRow(topLevelRow, new int[][] {{1, 1},
{0}}),
- equalTo(ROW(FIELD("a1_b1", thirdLevelRow), FIELD("a0",
INT()))));
+ assertThat(DataTypeUtils.projectRow(topLevelRow, new int[][] {{1, 1},
{0}}))
+ .isEqualTo(ROW(FIELD("a1_b1", thirdLevelRow), FIELD("a0",
INT())));
assertThat(
- DataTypeUtils.projectRow(
- topLevelRow, new int[][] {{1, 1, 2}, {1, 1, 1}, {1, 1,
0}}),
- equalTo(
+ DataTypeUtils.projectRow(
+ topLevelRow, new int[][] {{1, 1, 2}, {1, 1,
1}, {1, 1, 0}}))
+ .isEqualTo(
ROW(
FIELD("a1_b1_c2", INT()),
FIELD("a1_b1_c1", DOUBLE()),
- FIELD("a1_b1_c0", BOOLEAN()))));
+ FIELD("a1_b1_c0", BOOLEAN())));
- assertThat(
- DataTypeUtils.projectRow(topLevelRow, new int[][] {{1, 1, 0},
{2}}),
- equalTo(ROW(FIELD("a1_b1_c0", BOOLEAN()), FIELD("a1_b1_c0_$0",
INT()))));
+ assertThat(DataTypeUtils.projectRow(topLevelRow, new int[][] {{1, 1,
0}, {2}}))
+ .isEqualTo(ROW(FIELD("a1_b1_c0", BOOLEAN()),
FIELD("a1_b1_c0_$0", INT())));
}
@Test
public void testAppendRowFields() {
- {
- final DataType row =
- ROW(FIELD("a0", BOOLEAN()), FIELD("a1", DOUBLE()),
FIELD("a2", INT()));
-
- final DataType expectedRow =
- ROW(
- FIELD("a0", BOOLEAN()),
- FIELD("a1", DOUBLE()),
- FIELD("a2", INT()),
- FIELD("a3", BIGINT()),
- FIELD("a4", TIMESTAMP(3)));
-
- assertThat(
- DataTypeUtils.appendRowFields(
- row, Arrays.asList(FIELD("a3", BIGINT()),
FIELD("a4", TIMESTAMP(3)))),
- equalTo(expectedRow));
- }
-
- {
- final DataType row = ROW();
-
- final DataType expectedRow = ROW(FIELD("a", BOOLEAN()), FIELD("b",
INT()));
-
- assertThat(
- DataTypeUtils.appendRowFields(
- row, Arrays.asList(FIELD("a", BOOLEAN()),
FIELD("b", INT()))),
- equalTo(expectedRow));
- }
+ assertThat(
+ DataTypeUtils.appendRowFields(
+ ROW(
+ FIELD("a0", BOOLEAN()),
+ FIELD("a1", DOUBLE()),
+ FIELD("a2", INT())),
+ Arrays.asList(FIELD("a3", BIGINT()),
FIELD("a4", TIMESTAMP(3)))))
+ .isEqualTo(
+ ROW(
+ FIELD("a0", BOOLEAN()),
+ FIELD("a1", DOUBLE()),
+ FIELD("a2", INT()),
+ FIELD("a3", BIGINT()),
+ FIELD("a4", TIMESTAMP(3))));
+
+ assertThat(
+ DataTypeUtils.appendRowFields(
+ ROW(), Arrays.asList(FIELD("a", BOOLEAN()),
FIELD("b", INT()))))
+ .isEqualTo(ROW(FIELD("a", BOOLEAN()), FIELD("b", INT())));
}
+ private static final Condition<DataType> INTERNAL =
+ new Condition<>(DataTypeUtils::isInternal, "internal");
+
@Test
public void testIsInternalClass() {
- assertTrue(DataTypeUtils.isInternal(DataTypes.INT()));
-
assertTrue(DataTypeUtils.isInternal(DataTypes.INT().notNull().bridgedTo(int.class)));
-
assertTrue(DataTypeUtils.isInternal(DataTypes.ROW().bridgedTo(RowData.class)));
- assertFalse(DataTypeUtils.isInternal(DataTypes.ROW()));
+ assertThat(DataTypes.INT()).is(INTERNAL);
+
assertThat(DataTypes.INT().notNull().bridgedTo(int.class)).is(INTERNAL);
Review comment:
I think in general it's a good idea to use assertj `Condition`s more
than custom assert methods when there is a non parametrized property to test on
a specific object, and their failure gives no particular error message.
`Condition`s tend to be more flexible, because you can negate them, combine
them, exactly as Java's `Predicate`. A couple of examples from this PR that
explains my criteria:
* `isJavaSerializable` is a method because the failure gives some important
indication
* `hasSerializableString` is a method because it's parametrized (hence we
need its inverse)
* `INTERNAL` is a condition, because it's just a property check without any
useful error message in case the check fails
For some properties, i think it makes sense to have both a `Condition` and a
method in the assert class, for other very specific ones (like this
`INTERNAL`), it makes sense to have only a `Condition`.
I pushed a commit that adds two classes where we can have `Conditions` both
for `DataType` and `LogicalType`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
