Minor: name some constant values in arrow writer, parquet writer #8642
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
LGTM. I believe the parallel parquet writer has the same two constants as well. |
comphead
reviewed
Dec 24, 2023
| @@ -193,6 +193,13 @@ impl DisplayAs for ArrowFileSink { | |||
| } | |||
| } | |||
|
|
|||
| /// Initial writing buffer size. Note this is just a size hint for efficiency. It | |||
| /// will grow beyond the set value if needed. | |||
| const INITIAL_BUFFER_BYTES: usize = 1048576; | |||
There was a problem hiding this comment.
Thanks for documenting that, yesterday was pondering what those values exactly mean
there are more to go
There was a problem hiding this comment.
Filed follow on PR #8656
Update: since this PR wasn't yet approved, I'll just push commits here
alamb
changed the title
alamb
commented
Dec 26, 2023
| @@ -75,6 +75,17 @@ use crate::physical_plan::{ | |||
| Statistics, | |||
| }; | |||
|
|
|||
| /// Size of the buffer for [`AsyncArrowWriter`]. | |||
| const PARQUET_WRITER_BUFFER_SIZE: usize = 10485760; | |||
There was a problem hiding this comment.
I don't know why these values are (slightly) different, but I figured we could start by keeping them the same and then could unify them as a follow on if needed
andygrove
approved these changes
Dec 27, 2023
|
Thank you @andygrove |
appletreeisyellow
pushed a commit
to appletreeisyellow/datafusion
that referenced
this pull request
Jan 4, 2024
…che#8642) * Minor: name some constant values in arrow writer * Add constants to parquet.rs, update doc comments * fix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Follow on to #8608
Rationale for this change
The presence of numbers in the source code was not immediately obvious to me and I think it is clearer when there are comments to guide understanding. See #8608 (comment)
What changes are included in this PR?
Give two constants a name and add some docstrings to explain what they do
Are these changes tested?
Are there any user-facing changes?