openinx commented on a change in pull request #1956:
URL: https://github.com/apache/iceberg/pull/1956#discussion_r566791767
##########
File path:
flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java
##########
@@ -107,7 +107,7 @@ public void testRenameTable() {
() -> getTableEnv().from("tl")
);
Assert.assertEquals(
- Collections.singletonList(TableColumn.of("id", DataTypes.BIGINT())),
+ Collections.singletonList(TableColumn.physical("id",
DataTypes.BIGINT())),
Review comment:
Here, we could use the similar way to avoid to use
TableColumn#physical.
https://github.com/openinx/incubator-iceberg/commit/2a635719955f3eecd92685cb8f5486c46aa60095#diff-4c56ab08c19464dbe3351f71fc39345ee031a282b3e8dc1b107cbe9a1964d105R102
##########
File path: flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java
##########
@@ -62,9 +61,8 @@ private TestHelpers() {
}
public static RowData copyRowData(RowData from, RowType rowType) {
- ExecutionConfig config = new ExecutionConfig();
TypeSerializer[] fieldSerializers = rowType.getChildren().stream()
- .map((LogicalType type) -> InternalSerializers.create(type, config))
+ .map((LogicalType type) -> InternalSerializers.create(type))
Review comment:
Here we will use this to copy a `RowData` into a newly created
`GenericRowData`, that means we don't have to use the complex `TypeSerializer`
( that is used for copying binary data for `BinaryRowData`). I think it will
be good to remove the internal API if possible, so I'd recommend to use the
similar `RowDataConverter` way to copy a totally new java objects for each
fields.
BTW, I've discussed with flink sql team (@wuchong), the future apache
flink will provides RowData copy utility to acomplish this RowData clone..
##########
File path: flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java
##########
@@ -62,9 +61,8 @@ private TestHelpers() {
}
public static RowData copyRowData(RowData from, RowType rowType) {
- ExecutionConfig config = new ExecutionConfig();
TypeSerializer[] fieldSerializers = rowType.getChildren().stream()
- .map((LogicalType type) -> InternalSerializers.create(type, config))
+ .map((LogicalType type) -> InternalSerializers.create(type))
Review comment:
Of course, we could open separate PR to address the
`InternalSerializers` issue if you think it's necessary.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
