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

Replace i64 with DateTime #94

Merged
merged 4 commits into from
Nov 22, 2023
Merged

Conversation

fqaiser94
Copy link
Contributor

@fqaiser94 fqaiser94 commented Nov 11, 2023

Fixes: #90

First time contributor to this repo.
Extremely new to rust (not new to iceberg, used it extensively and contributed a couple of small things there).

How is this for an approach?
I used chrono::DateTime<Utc> but had to wrap it in a NewType to implement the Serialize and Deserialize traits (as otherwise we run into Rust's orphan rule issue).

@fqaiser94 fqaiser94 marked this pull request as draft November 13, 2023 23:33
@fqaiser94
Copy link
Contributor Author

Moving back to draft while i address comments. Will ping when it's ready again, shouldn't take too long!

@fqaiser94 fqaiser94 marked this pull request as ready for review November 15, 2023 01:18
Copy link
Collaborator

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks for this pr. I've left some comments.

crates/iceberg/src/spec/timestamp_millis.rs Outdated Show resolved Hide resolved
crates/iceberg/src/spec/snapshot.rs Outdated Show resolved Hide resolved
crates/iceberg/src/spec/snapshot.rs Outdated Show resolved Hide resolved
crates/iceberg/src/spec/table_metadata.rs Outdated Show resolved Hide resolved
crates/iceberg/src/spec/timestamp_millis.rs Outdated Show resolved Hide resolved
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

Copy link
Collaborator

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fqaiser94
Copy link
Contributor Author

Is there anything else I need to do to get this merged in?

I don't (understandably) have permissions to merge this myself.
Screen Shot 2023-11-19 at 10 58 27 PM

@liurenjie1024
Copy link
Collaborator

Is there anything else I need to do to get this merged in?

I don't (understandably) have permissions to merge this myself. Screen Shot 2023-11-19 at 10 58 27 PM

cc @Fokko PTAL

@Fokko
Copy link
Contributor

Fokko commented Nov 20, 2023

Looks like we have a sad clippy:

error: argument to `panic!()` in a const context must have type `&str`
   --> crates/catalog/rest/src/catalog.rs:985:13
    |
985 |             uuid!("b55d9dda-6561-423a-8bfc-787980ce421f"),
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `uuid` (in Nightly builds, run with -Z macro-backtrace for more info)

@liurenjie1024
Copy link
Collaborator

liurenjie1024 commented Nov 20, 2023

The failure is caused by upgrades of uuid. You can solve it by fixing version as this pr:

image

cc @fqaiser94

@liurenjie1024
Copy link
Collaborator

cc @Fokko Any other comments?

@Fokko Fokko merged commit 497a1b5 into apache:main Nov 22, 2023
6 checks passed
@Fokko
Copy link
Contributor

Fokko commented Nov 22, 2023

Thanks @fqaiser94 for the PR, and @liurenjie1024 & @Xuanwo for the review 🙌

@fqaiser94 fqaiser94 mentioned this pull request Nov 25, 2023
@fqaiser94 fqaiser94 deleted the 90_replace_i64_with_datetime branch November 25, 2023 03:32
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.

Substitue in memory data struct's timestamp type for DataTime rather i64 to simplify usage.
4 participants