Skip to content

[FLINK-25565][Formats][Parquet] write and read parquet int64 timestamp#23887

Merged
tweise merged 1 commit into
apache:release-1.18from
tweise:release-1.18_parquet
Dec 8, 2023
Merged

[FLINK-25565][Formats][Parquet] write and read parquet int64 timestamp#23887
tweise merged 1 commit into
apache:release-1.18from
tweise:release-1.18_parquet

Conversation

@tweise

@tweise tweise commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

backport #18304

@flinkbot

flinkbot commented Dec 6, 2023

Copy link
Copy Markdown
Collaborator

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@MartijnVisser

MartijnVisser commented Dec 7, 2023

Copy link
Copy Markdown
Contributor

@tweise Was this a bug? If so, we should change the type of ticket. Don't forget to add 1.18.1 as a fix version then as well.

Edit: Ah it's a subtask, so you can't have a bug type for that.

@tweise

tweise commented Dec 7, 2023

Copy link
Copy Markdown
Contributor Author

@MartijnVisser good point - strictly speaking it is a bug, let me add a comment to the ticket.

@mbalassi mbalassi self-requested a review December 7, 2023 14:51

@mbalassi mbalassi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving it based on #18304, as the diff is the same. Thanks for the backport, @tweise.

nanosOfMillisecond = value % NANOS_PER_SECOND;
break;
default:
break;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No time unit means drop the value? Should we throw an error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. LogicalTypeAnnotation.TimeUnit has only the 3 values that are all covered, so there should neither be a practical danger of dropping the value nor big risk of throwing an exception under default for clarity. It is also more of a stylistic concern that we can deal with in the master branch. I will open a PR there.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would throw an exception for defensive programming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants