-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
[native] Fix protocol::FileFormat::ORC mapping to use dwio::common::FileFormat::ORC #22726
Conversation
a5194dc
to
4fa7aba
Compare
|
3202320
to
2df47c3
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.
@wypb The queries all failed, have you taken a look what caused it? Was it Iceberg related issue?
No, This is due to the fact that the open source Velox does not support reading ORC files(some adaptation is required: facebookincubator/velox#6588). I made an adaptation internally, and I need to remove this part of the code in this PR. |
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.
@wypb This is not limited to the Iceberg connector correct? This is a general protocol::FileFormat mapping issue.
Can you update the commit title and add a description?
I am also assuming the current mapping is a bug. |
@majetideepak Currently,
I've updated the commit title and added a description, Thank you. |
…ileFormat::ORC Currently, Prestissimo's toVeloxFileFormat method maps the ORC file format to Velox's DWRF file format, and an error will occur when reading the Iceberg table in the ORC file format.
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, @wypb
Description
Iceberg's ORC format should be mapped to Velox's ORC format, not the DWRF format.
Because the open source Velox does not currently support reading ORC files (some adaptation is required), adding tests to read ORC will fail on the native side. So I didn't add the test to read ORC.
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes