You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/07/07 05:50:36 UTC
[GitHub] [iceberg] JingsongLi opened a new pull request #1174: Add TypeToFlinkType: convert iceberg types to Flink types
JingsongLi opened a new pull request #1174:
URL: https://github.com/apache/iceberg/pull/1174
Converter between Flink types and Iceberg type.
The conversion is not a 1:1 mapping that not allows back-and-forth conversion. So some information might get lost during the back-and-forth conversion.
(background: FlinkCatalog need convert iceberg tables to Flink tables)
----------------------------------------------------------------
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:
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] openinx commented on a change in pull request #1174: Add TypeToFlinkType: convert iceberg types to Flink types
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #1174:
URL: https://github.com/apache/iceberg/pull/1174#discussion_r452119193
##########
File path: flink/src/test/java/org/apache/iceberg/flink/TestFlinkSchemaUtil.java
##########
@@ -218,6 +218,24 @@ public void testListField() {
))
);
- Assert.assertEquals(expectedSchema.asStruct(), actualSchema.asStruct());
+ checkSchema(flinkSchema, icebergSchema);
+ }
+
+ private void checkSchema(TableSchema flinkSchema, Schema icebergSchema) {
+ Assert.assertEquals(icebergSchema.asStruct(), FlinkSchemaUtil.convert(flinkSchema).asStruct());
+ // The conversion is not a 1:1 mapping, so we just check iceberg types.
+ Assert.assertEquals(
+ icebergSchema.asStruct(),
+ FlinkSchemaUtil.convert(FlinkSchemaUtil.toSchema(FlinkSchemaUtil.convert(icebergSchema))).asStruct());
Review comment:
I'd prefer to provide some separate unit tests to address the conversion differences ( so that we won't regress).
----------------------------------------------------------------
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:
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] rdblue commented on pull request #1174: Add TypeToFlinkType: convert iceberg types to Flink types
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1174:
URL: https://github.com/apache/iceberg/pull/1174#issuecomment-658306689
Looks good to me. The test failure is from Spark tests taking too long, #1198. Hopefully the next run passes when this is rebased. It had conflicts with #1173.
----------------------------------------------------------------
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:
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] rdblue closed pull request #1174: Add TypeToFlinkType: convert iceberg types to Flink types
Posted by GitBox <gi...@apache.org>.
rdblue closed pull request #1174:
URL: https://github.com/apache/iceberg/pull/1174
----------------------------------------------------------------
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:
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] JingsongLi commented on a change in pull request #1174: Add TypeToFlinkType: convert iceberg types to Flink types
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on a change in pull request #1174:
URL: https://github.com/apache/iceberg/pull/1174#discussion_r453405611
##########
File path: flink/src/test/java/org/apache/iceberg/flink/TestFlinkSchemaUtil.java
##########
@@ -218,6 +218,24 @@ public void testListField() {
))
);
- Assert.assertEquals(expectedSchema.asStruct(), actualSchema.asStruct());
+ checkSchema(flinkSchema, icebergSchema);
+ }
+
+ private void checkSchema(TableSchema flinkSchema, Schema icebergSchema) {
+ Assert.assertEquals(icebergSchema.asStruct(), FlinkSchemaUtil.convert(flinkSchema).asStruct());
+ // The conversion is not a 1:1 mapping, so we just check iceberg types.
+ Assert.assertEquals(
+ icebergSchema.asStruct(),
+ FlinkSchemaUtil.convert(FlinkSchemaUtil.toSchema(FlinkSchemaUtil.convert(icebergSchema))).asStruct());
Review comment:
I'll add tests for the conversion differences.
----------------------------------------------------------------
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:
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] rdblue commented on pull request #1174: Add TypeToFlinkType: convert iceberg types to Flink types
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1174:
URL: https://github.com/apache/iceberg/pull/1174#issuecomment-658988091
Thanks for rebasing! CI has been fixed in master so I reopened this to run tests. They are passing, so I'll merge.
----------------------------------------------------------------
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:
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] JingsongLi commented on pull request #1174: Add TypeToFlinkType: convert iceberg types to Flink types
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on pull request #1174:
URL: https://github.com/apache/iceberg/pull/1174#issuecomment-657934757
Synchronized with #1182.
----------------------------------------------------------------
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:
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] rdblue commented on pull request #1174: Add TypeToFlinkType: convert iceberg types to Flink types
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1174:
URL: https://github.com/apache/iceberg/pull/1174#issuecomment-656946005
@JingsongLi, I added some comments about the changes here to #1182.
----------------------------------------------------------------
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:
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] rdblue merged pull request #1174: Add TypeToFlinkType: convert iceberg types to Flink types
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1174:
URL: https://github.com/apache/iceberg/pull/1174
----------------------------------------------------------------
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:
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] JingsongLi commented on a change in pull request #1174: Add TypeToFlinkType: convert iceberg types to Flink types
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on a change in pull request #1174:
URL: https://github.com/apache/iceberg/pull/1174#discussion_r454745582
##########
File path: flink/src/main/java/org/apache/iceberg/flink/FlinkSchemaUtil.java
##########
@@ -21,10 +21,30 @@
import org.apache.flink.table.api.TableSchema;
import org.apache.flink.table.types.FieldsDataType;
+import org.apache.flink.table.types.logical.LogicalType;
+import org.apache.flink.table.types.logical.RowType;
+import org.apache.flink.table.types.utils.TypeConversions;
import org.apache.iceberg.Schema;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+/**
+ * Converter between Flink types and Iceberg type.
+ * The conversion is not a 1:1 mapping that not allows back-and-forth conversion. So some information might get lost
+ * during the back-and-forth conversion.
+ * <p>
+ * This inconsistent types:
+ * <ul>
+ * <li>map Iceberg UUID type to Flink BinaryType(16)</li>
+ * <li>map Flink VarCharType and CharType to Iceberg String type (lost precision)</li>
+ * <li>map Flink VarBinaryType to Iceberg Binary type (lost precision)</li>
+ * <li>map Flink TimeType to Iceberg Time type (lost precision)</li>
+ * <li>map Flink TimestampType to Iceberg Timestamp without zone type (lost precision)</li>
+ * <li>map Flink LocalZonedTimestampType to Iceberg Timestamp with zone type (lost precision)</li>
Review comment:
Yes, I'll modify comments.
----------------------------------------------------------------
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:
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] rdblue commented on a change in pull request #1174: Add TypeToFlinkType: convert iceberg types to Flink types
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1174:
URL: https://github.com/apache/iceberg/pull/1174#discussion_r454513800
##########
File path: flink/src/main/java/org/apache/iceberg/flink/FlinkSchemaUtil.java
##########
@@ -21,10 +21,30 @@
import org.apache.flink.table.api.TableSchema;
import org.apache.flink.table.types.FieldsDataType;
+import org.apache.flink.table.types.logical.LogicalType;
+import org.apache.flink.table.types.logical.RowType;
+import org.apache.flink.table.types.utils.TypeConversions;
import org.apache.iceberg.Schema;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+/**
+ * Converter between Flink types and Iceberg type.
+ * The conversion is not a 1:1 mapping that not allows back-and-forth conversion. So some information might get lost
+ * during the back-and-forth conversion.
+ * <p>
+ * This inconsistent types:
+ * <ul>
+ * <li>map Iceberg UUID type to Flink BinaryType(16)</li>
+ * <li>map Flink VarCharType and CharType to Iceberg String type (lost precision)</li>
+ * <li>map Flink VarBinaryType to Iceberg Binary type (lost precision)</li>
+ * <li>map Flink TimeType to Iceberg Time type (lost precision)</li>
+ * <li>map Flink TimestampType to Iceberg Timestamp without zone type (lost precision)</li>
+ * <li>map Flink LocalZonedTimestampType to Iceberg Timestamp with zone type (lost precision)</li>
Review comment:
Precision isn't lost for date/time types. It is always microseconds.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org