-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
GH-38887: [C++][Parquet] Move EstimatedBufferedValueBytes from TypedColumnWriter to ColumnWriter #39055
GH-38887: [C++][Parquet] Move EstimatedBufferedValueBytes from TypedColumnWriter to ColumnWriter #39055
Conversation
@pitrou @wgtmac Also cc @Hattonuri |
cpp/src/parquet/column_writer.h
Outdated
@@ -175,6 +175,9 @@ class PARQUET_EXPORT ColumnWriter { | |||
/// total_bytes_written(). | |||
virtual int64_t total_compressed_bytes_written() const = 0; | |||
|
|||
/// \brief Estimated size of the values that are not written to a page yet. | |||
virtual int64_t EstimatedBufferedValueBytes() const = 0; |
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.
It is unrelated, but the naming looks weird after relocation...
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.
+1
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.
+1 on my side. But this introduces a minor breaking change.
Though |
IMO, this is really a minor breaking change and it is very easy for users to fix. So I don't think we need to add a wrapper or something. |
Merged, cc @Hattonuri |
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 1cc1f4c. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…TypedColumnWriter to ColumnWriter (apache#39055) ### Rationale for this change Trying to put `EstimatedBufferedValueBytes` from `TypedColumnWriter` to `ColumnWriter`. ### What changes are included in this PR? put `EstimatedBufferedValueBytes` from `TypedColumnWriter` to `ColumnWriter`. ### Are these changes tested? No, just interface change ### Are there any user-facing changes? Yes, interface changed * Closes: apache#38887 Authored-by: mwish <maplewish117@gmail.com> Signed-off-by: mwish <maplewish117@gmail.com>
Rationale for this change
Trying to put
EstimatedBufferedValueBytes
fromTypedColumnWriter
toColumnWriter
.What changes are included in this PR?
put
EstimatedBufferedValueBytes
fromTypedColumnWriter
toColumnWriter
.Are these changes tested?
No, just interface change
Are there any user-facing changes?
Yes, interface changed