-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(parquet): Add Int64 Timestamp in reader #11530
Conversation
✅ Deploy Preview for meta-velox canceled.
|
Could you add some unit tests? |
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.
@zuyu Can you please add a commit and PR message describing the change?
I am assuming you're also planning to add support for Dictionary encoding and range filters, currently the range filter is always casted to INT96 range filter. |
2c824cd
to
43dc8a9
Compare
376876c
to
c8e2270
Compare
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.
Thanks! I added some comments.
@rui-mo Addressed ur comments. Could u take another look? The test failures are the same as the |
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.
Thanks! Added some nits and overall looks good to me.
@@ -813,6 +813,11 @@ TypePtr ReaderBase::convertType( | |||
case thrift::Type::type::INT32: | |||
return INTEGER(); | |||
case thrift::Type::type::INT64: | |||
// For Int64 Timestamp in nano precision |
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.
in nano precision
nit: is other precision other than nano possible?
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.
No, other precisions handle separately: see #11600 (comment)
@zacw7 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
w/ both plain and dictionary encoding support.