Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#2117] improvement(api): Type adds UNPARSED column data type to handle an unresolvable type from the catalog #2140

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

SteNicholas
Copy link
Member

@SteNicholas SteNicholas commented Feb 7, 2024

What changes were proposed in this pull request?

Type adds UNPARSED column data type to handle an unresolvable type from the catalog.

Why are the changes needed?

If a column data type in the catalog does not have a mapping in Gravitino, an exception will be thrown. We should map all data types from a catalog to Gravitino. However, in practical situations, loading exceptions may occur due to the addition of new data types or insufficient mapping support. Introduce UnparsedType to represent the unparsed data type.

Fix: #2117

Does this PR introduce any user-facing change?

Type adds UNKNOWN column data type that represents an unresolvable type.

How was this patch tested?

  • TestJsonUtils#testTypeSerDe
  • TestTypes#testUnparsedType
  • TestTypeConverter#testTypeConverter
  • TestMysqlTypeConverter#testToGravitinoType
  • TestPostgreSqlTypeConverter#testToGravitinoType
  • CatalogMysqlIT#testUnparsedTypeConverter
  • CatalogPostgreSqlIT#testUnparsedTypeConverter

@SteNicholas
Copy link
Member Author

cc @mchades, @FANNG1

@FANNG1 FANNG1 requested a review from jerryshao February 8, 2024 06:32
Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

Missing JSON SerDe for Unknown type

@SteNicholas SteNicholas changed the title [#2117] improvement(api): Type adds UNKNOWN column data type to handle an unresolvable type from the catalog [#2117] improvement(api): Type adds UNPARSED column data type to handle an unresolvable type from the catalog Feb 22, 2024
@SteNicholas SteNicholas force-pushed the GRAVITINO-2117 branch 2 times, most recently from f98ca68 to da04866 Compare February 22, 2024 09:25
@SteNicholas SteNicholas requested a review from mchades February 22, 2024 09:26
@SteNicholas SteNicholas reopened this Feb 22, 2024
@SteNicholas
Copy link
Member Author

SteNicholas commented Feb 22, 2024

@mchades, @FANNG1, I have addressed comments of #2117 and introduced UnparsedType for UNPARSED data type. PTAL.

@mchades
Copy link
Contributor

mchades commented Feb 22, 2024

Could you please add related integration tests?

@SteNicholas SteNicholas force-pushed the GRAVITINO-2117 branch 5 times, most recently from 167bb0e to 007f184 Compare February 22, 2024 17:49
@SteNicholas SteNicholas reopened this Feb 22, 2024
@SteNicholas
Copy link
Member Author

@mchades, I have added CatalogMysqlIT#testUnparsedTypeConverter and CatalogPostgreSqlIT#testUnparsedTypeConverter. PTAL.

@mchades
Copy link
Contributor

mchades commented Feb 23, 2024

Overall LGTM, but missing support for Iceberg catalog, you need to modify com.datastrato.gravitino.catalog.lakehouse.iceberg.converter.FromIcebergType

…o handle an unresolvable type from the catalog
@SteNicholas
Copy link
Member Author

@mchades, I have modified FromIcebergType for UnparsedType. PTAL.

@xunliu
Copy link
Member

xunliu commented Feb 23, 2024

@SteNicholas
Thank you for your contribution.
If no more comments, I will merge this PR.

@mchades
Copy link
Contributor

mchades commented Feb 23, 2024

The document related work will be tracked by #2323

@mchades mchades merged commit e2a7ea1 into apache:main Feb 23, 2024
12 checks passed
mchades pushed a commit that referenced this pull request Feb 28, 2024
…solvable type from the catalog (#2337)

### What changes were proposed in this pull request?

Document `UnparsedType` data type which handles an unresolvable type
from the catalog.

### Why are the changes needed?

`UnparsedType` data type that handles an unresolvable type from the
catalog is supported in #2140.

Fix: #2323

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

No.
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.

[Improvement] Add Unknown column data type to handle an unresolvable type from the catalog
3 participants