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

[core]IcebergConversions addType for Timestamp and Time #3866

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

dbac
Copy link

@dbac dbac commented Aug 1, 2024

Purpose

Linked issue: close #3788

IcebergConversions addType for Timestamp and Time

Tests

IcebergDataTypeCompatibilityTest

API and Format

Documentation

@tsreaper tsreaper self-requested a review August 2, 2024 06:48
@dbac
Copy link
Author

dbac commented Aug 5, 2024

PostgresSyncTableActionITCase.testOptionsChange » Timeout testOptionsChange()
This timeout check failed, please tell me how to re-check, thank you very much
@tsreaper

@dbac
Copy link
Author

dbac commented Aug 7, 2024

@tsreaper please Review code again,thanks

@dbac
Copy link
Author

dbac commented Aug 9, 2024

@tsreaper Could you please help me take a look when you have time? Thank you

# Conflicts:
#	paimon-core/src/main/java/org/apache/paimon/iceberg/manifest/IcebergConversions.java
#	paimon-core/src/test/java/org/apache/paimon/iceberg/IcebergCompatibilityTest.java
# Conflicts:
#	paimon-core/src/test/java/org/apache/paimon/iceberg/IcebergCompatibilityTest.java
@dbac
Copy link
Author

dbac commented Aug 27, 2024

@tsreaper I am reading the source code now and solved this problem. Please review it for me.

@dbac
Copy link
Author

dbac commented Aug 27, 2024

I found that a lot of the core code was written by you. I will learn from you. I think the source code should make me familiar with paimon quickly. I hope this PR will be submitted as soon as possible. I will continue to contribute to the next one.

@tsreaper
Copy link
Contributor

Hi @dbac , sorry for keeping you waiting. I've just introduced statistics to IcebergDataFileMeta, so now Iceberg will filter files according to the statistics we converted from IcebergConversions, and we can test the conversion reliably.

Please resolve the conflicts and add tests about timestamp and time to IcebergCompatibilityTest#testAllTypeStatistics. More specifically, do not add new test method. Just add a few columns with timestamp and time type to that test.

# Conflicts:
#	paimon-core/src/test/java/org/apache/paimon/iceberg/IcebergCompatibilityTest.java
@dbac
Copy link
Author

dbac commented Aug 29, 2024

@tsreaper
image
time only support 3 pecesion (so, ms ), and use int writer.

so IcebergConversions time value should int not long.

@dbac
Copy link
Author

dbac commented Aug 29, 2024

image

@dbac
Copy link
Author

dbac commented Aug 30, 2024

@tsreaper
image
Is it a bug? Lessthan invalid, greaterthan ok ?
Is there a problem with writing iceberg data?

@dbac
Copy link
Author

dbac commented Aug 30, 2024

apache/iceberg#9431

@dbac
Copy link
Author

dbac commented Sep 6, 2024

@tsreaper ? Could you help me review it again?

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.

[Feature][core] Implement more type conversions for IcebergConversions
2 participants