-
Notifications
You must be signed in to change notification settings - Fork 452
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
[GLUTEN-8307][VL] Support Int64 Timestamp in parquet reader #8308
Conversation
Run Gluten Clickhouse CI on x86 |
CC @rui-mo |
Abnormal UT:
cc @rui-mo do you know the reason? |
@zml1206 I'm glad to investigate them this week. Do you think it makes sense to hold on this PR for a while? |
Hi @zml1206, I opened facebookincubator/velox#11964 to address above failed case. And I assume we can enable this unit test after the PR being merged.
This case doesn't seem to be relevant to the timestamp reader. I suggest we not enabling it in this PR. Thanks. |
facebookincubator/velox#11651, Is it the same problem? @rui-mo |
@zml1206 Let me pick that PR to check if it is solved. Thanks for the pointer. |
@zml1206 I picked that PR but strangely the test I suggest we do not enable this test in this PR for the reasons: 1) it is not timestamp related. 2) it depends on another in-progress PR. facebookincubator/velox#11964 has been merged, and the other timestamp tests could be enabled. |
OK, I'll do it, thank you. |
Run Gluten Clickhouse CI on x86 |
@rui-mo Updated, can you help take.a look? thank you. |
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.
Looks good! Thanks.
What changes were proposed in this pull request?
facebookincubator/velox#11530 add Int64 Timestamp in parquet reader, remove config VELOX_SCAN_FILE_SCHEME_VALIDATION_ENABLED and enable related ut.
(Fixes: #8307)
How was this patch tested?