You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "etseidl (via GitHub)" <gi...@apache.org> on 2025/11/04 22:51:50 UTC
[PR] Log type ref [arrow-rs]
etseidl opened a new pull request, #8786:
URL: https://github.com/apache/arrow-rs/pull/8786
# Which issue does this PR close?
- Partial fix for #8783.
# Rationale for this change
An appreciable slowdown in Parquet writing was noticed. At least part of the slowdown seems to stem from changes to `basic::LogicalType` which caused cloning the enum to take make longer than previously.
# What changes are included in this PR?
This adds a new `logical_type_ref` call to `BasicTypeInfo` and `ColumnDescriptor`. Unlike the existing `logical_type` which returns a cloned `Option<LogicalType>`, the new methods return `Option<&LogicalType>`. This new function is used in place of `logical_type` internally.
# Are these changes tested?
Should be covered by existing tests.
# Are there any user-facing changes?
No.
A further optimization would be to change `ColumnOrder::get_sort_order` to take an `Option<&LogicalType>`, but that is a breaking API change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: Speed up Parquet file writing (10%, back to speed of 56) [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl merged PR #8786:
URL: https://github.com/apache/arrow-rs/pull/8786
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: Speed up Parquet file writing (10%, back to speed of 56) [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8786:
URL: https://github.com/apache/arrow-rs/pull/8786#issuecomment-3493375555
Given #8789 is open I don't think this needs any further work. Thanks again for the quick reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: Speed up Parquet file writing (10%, back to speed of 56) [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8786:
URL: https://github.com/apache/arrow-rs/pull/8786#issuecomment-3491797624
Thanks @mbrobbel and @alamb!
> I wonder if we should add a comment to LogicalType that it isn't trivially copy'able (and remove the Copy in later breaking release)?
I did add a note to the getters that the value is cloned. I just wonder do we want to deprecate the cloning getters? It would be nice if eventually `logical_type()` returned `Option<&LogicalType>` and we could remove `logical_type_ref()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: Speed up Parquet file writing (10%, back to speed of 56) [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on PR #8786:
URL: https://github.com/apache/arrow-rs/pull/8786#issuecomment-3491929441
> Thanks @mbrobbel and @alamb!
>
> > I wonder if we should add a comment to LogicalType that it isn't trivially copy'able (and remove the Copy in later breaking release)?
>
> I did add a note to the getters that the value is cloned. I just wonder do we want to deprecate the cloning getters? It would be nice if eventually `logical_type()` returned `Option<&LogicalType>` and we could remove `logical_type_ref()`.
+1 for `logical_type() -> Option<&LogicalType>`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: Speed up Parquet file writing (10%, back to speed of 56) [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8786:
URL: https://github.com/apache/arrow-rs/pull/8786#issuecomment-3491706494
I wonder if we should add a comment to `LogicalType` that it isn't trivially copy'able (and remove the `Copy` in later breaking release)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: Speed up Parquet file writing (10%, back to speed of 56) [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8786:
URL: https://github.com/apache/arrow-rs/pull/8786#issuecomment-3492016855
> What would the mechanics of such a change be? Do we deprecate with a note that the API will change in a future release? Or do we just change the signature in 58.0.0 and note the breaking change?
I think deprecate with a note is a good idea
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: Speed up Parquet file writing (10%, back to speed of 56) [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8786:
URL: https://github.com/apache/arrow-rs/pull/8786#issuecomment-3492032404
> I tried this out and didn't see any noticable performance change in my test, but I will make a PR anyways to discuss
https://github.com/apache/arrow-rs/pull/8789
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: Speed up Parquet file writing (10%, back to speed of 56) [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8786:
URL: https://github.com/apache/arrow-rs/pull/8786#issuecomment-3491995977
> +1 for `logical_type() -> Option<&LogicalType>`
What would the mechanics of such a change be? Do we deprecate with a note that the API will change in a future release? Or do we just change the signature in `58.0.0` and note the breaking change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org