-
Notifications
You must be signed in to change notification settings - Fork 175
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
[BUG] Bump up max_header_size #3068
Conversation
CodSpeed Performance ReportMerging #3068 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3068 +/- ##
==========================================
- Coverage 78.65% 77.20% -1.45%
==========================================
Files 618 618
Lines 73192 75100 +1908
==========================================
+ Hits 57569 57983 +414
- Misses 15623 17117 +1494
|
src/daft-parquet/src/file.rs
Outdated
const MAX_HEADER_SIZE: usize = 256 * 1024 * 1024; | ||
const MAX_HEADER_SIZE: usize = 2 * 1024 * 1024 * 1024; |
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.
Could we update the comment above this to explain our rationale for using 2GiB?
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.
Yessir! 🚀🚀🚀
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! Send it
Hey what's happening with this PR? Is this good to merge? |
@jaychia Ingress works, but currently debugging egress with PyArrow. |
Overview
Bump up the
MAX_HEADER_SIZE
constant that's being used to compare against the page sizes.(Naming seems weird. We're using the max "header" size to guard against "page" sizes.)