Skip to content

Conversation

@JingsongLi
Copy link
Contributor

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)

// 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());
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to provide some separate unit tests to address the conversion differences ( so that we won't regress).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add tests for the conversion differences.

@rdblue
Copy link
Contributor

rdblue commented Jul 11, 2020

@JingsongLi, I added some comments about the changes here to #1182.

@JingsongLi
Copy link
Contributor Author

Synchronized with #1182.

@rdblue
Copy link
Contributor

rdblue commented Jul 14, 2020

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.

@rdblue rdblue closed this Jul 15, 2020
@rdblue rdblue reopened this Jul 15, 2020
@rdblue
Copy link
Contributor

rdblue commented Jul 15, 2020

Thanks for rebasing! CI has been fixed in master so I reopened this to run tests. They are passing, so I'll merge.

@rdblue rdblue merged commit 2f69c9c into apache:master Jul 15, 2020
HotSushi pushed a commit to HotSushi/iceberg that referenced this pull request Jul 23, 2020
cmathiesen pushed a commit to ExpediaGroup/iceberg that referenced this pull request Aug 19, 2020
@JingsongLi JingsongLi deleted the typeToFlinkType branch November 5, 2020 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants