-
Notifications
You must be signed in to change notification settings - Fork 261
feat: add config option for skipping arrow ipc read validation #1374
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
Conversation
milenkovicm
left a comment
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 @killzoner
i think changes make sense.
My major comment for discussion if validation should be runtime configuration rather than compile time, I'm leaning towards later, but open for discussion
I pretty much replicated existing config setup, but a compile time config makes sense, as it's a strong default not supposed to be changed frequently. I will put this specific config under a new feature gate, part of the default features. If you have a name in mind don't hesitate 😄 |
|
Im bad with naming things 😁 |
ad0869e to
959248b
Compare
104b58e to
d54bbd3
Compare
|
Removed all unecessary parametrization, moved config to ShuffleReaderExec. Also duplicated the function for config in FlightService, since i was not sure it made sense to refer to Ballista core config for FlightService config |
milenkovicm
left a comment
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.
I have just two minor comments,
|
thanks @killzoner |
Thanks! Updated from your suggestions |
Which issue does this PR close?
Closes #1189.
Revival of #1216 with added configuration.
Rationale for this change
Being able to make use of optimized Arrow IPC reads
What changes are included in this PR?
arrow-ipc-optimizationsfeature (default enabled) to enable optioncargo neat)Are there any user-facing changes?
No