-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
PARQUET-2382: Remove the deprecated OriginalType
#1194
base: master
Are you sure you want to change the base?
Conversation
2176e3c
to
b182a41
Compare
OriginalType originalType = getLogicalTypeAnnotation(schemaElement.converted_type, schemaElement).toOriginalType(); | ||
OriginalType newOriginalType = (schemaElement.isSetLogicalType() && getLogicalTypeAnnotation(schemaElement.logicalType) != null) ? | ||
getLogicalTypeAnnotation(schemaElement.logicalType).toOriginalType() : null; | ||
if (!originalType.equals(newOriginalType)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was a 🐛 hidden here. Because we would only compare on the logical type itself, and not its properties (precision/scale for decimal, or adjust-for-utc for time/timestamp). Tests started failing, therefore I added the getAdjustToUtc
to retrieve the actual value from the Parquet structure.
OriginalType
I am fine with it but I'd like to seek advices from @gszadovszky @shangxinli |
I don't think it is a good idea to remove deprecated API in a minor release. That's why we have japicmp to ensure backward compatibility. |
Makes sense. I think it would be good to remove |
Make sure you have checked all steps below.
For Iceberg we're adding nanosecond timestamps. During my investigation in Parquet, I noticed that there are two ways of declaring logical types:
OriginalType
The old API does not support nano's but is still used in downstream projects, such as Parquet, where I'm working on migrating to the new API: apache/iceberg#9063
However, since it was five years ago in PARQUET-1452 when it was marked as deprecated, released in Parquet 1.11.0. I would love to remove the old API to make sure that downstream engines migrate to the new API and handle nano's correctly.
Jira
Tests
Commits
Documentation