-
Notifications
You must be signed in to change notification settings - Fork 849
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 record API: timestamp as signed integer #3437
Conversation
- Use signed integers to store 'Date', 'TimestampMillis' and 'TimestampMicros' in 'enum Field' - remove timezone from string representation of Date
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.
Thank you
static NUM_SECONDS_IN_DAY: i64 = 60 * 60 * 24; | ||
let dt = Utc | ||
.timestamp_opt(value as i64 * NUM_SECONDS_IN_DAY, 0) | ||
.unwrap(); | ||
format!("{}", dt.format("%Y-%m-%d %:z")) |
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.
Why is the %:z
removed? Is it because it is always zero?
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.
What does a timezone mean for a date?
Benchmark runs are scheduled for baseline = b82b35f and contender = 65ff80e. 65ff80e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Date
,TimestampMillis
andTimestampMicros
inenum Field
Which issue does this PR close?
Closes #3430 .
Rationale for this change
The
Field
type inparquet::record
interpreted Date/Time values as unsigned integers, representing number of seconds from Unix epoch. This was wrong because values before epoch do exist. When encountered with aRow
that contains such a value, callingto_json_value
panicked.What changes are included in this PR?
This commit changes the implementation of
Field
to now use signed integers to read timestamp. It also removes timezone from the string representation ofField::Date
, which made no sense otherwise.Are there any user-facing changes?
None. All test cases pass.