You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mbrobbel (via GitHub)" <gi...@apache.org> on 2025/09/15 09:11:40 UTC
[PR] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
mbrobbel opened a new pull request, #8350:
URL: https://github.com/apache/arrow-rs/pull/8350
# Which issue does this PR close?
- Closes #8030.
# Rationale for this change
As discussed in the linked issue we want to remove the `experimental` warning from the `extension` module.
When adding extension type information via the `Field` methods, `ExtensionType::supports_datatype` is used to validate the `Field` datatype is compatible with the given extension type. When users change the `Field` datatype, it's not clear if this is still true, so this PR proposes to remove the extension type information when changing datatype information.
# What changes are included in this PR?
Adds two `Field` methods:
- `Field::clear_extension_type`
- `Field::without_extension_type`
These methods are used when changing data types via `Field::set_data_type` and `Field::with_data_type`.
# Are these changes tested?
Yes, CI.
# Are there any user-facing changes?
Yes, `Field::set_data_type` and `Field::with_data_type` now clear extension type information from the `Field` metadata.
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357600500
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
This change seems really dangerous... it will silently strip away the extension type information, which can propagate into e.g. the parquet file the column eventually gets written to, which could be very hard to fix later on when downstream readers start noticing the extension type info is unexpectedly missing.
Seems better to make this fallible (`with_data_type` becomes `try_with_data_type`), and blow up if the new data type is incompatible with the currently-installed extension type?
We could potentially still keep the original (infallible) version and panic if the new type is invalid. Doing that means people upgrading to arrow-57 won't necessarily realize there's a potential problem until their code starts crashing, which I don't love, but I suspect some people would rather deal with a potential panic than have to rewrite all their `with_data_type` calls?
Alternatively -- maybe we leave the panicky infallible version in place, but deprecated with an appropriate warning? But people with automated builds won't necessarily _see_ the warning, especially if they already have lots of warnings in their code.
If somebody _actually_ wanted to clear the extension type, it seems much better to require them to clearly state their intent by e.g. `field.without_extension_type().with_data_type(...)`
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357600500
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
This change seems really dangerous... it will silently strip away the extension type information, which can propagate into e.g. the parquet file the column eventually gets written to, which could be very hard to fix later on once readers start noticing the extension type info is unexpectedly missing.
Seems better to make this fallible, and blow up if there is an extension type for which the new data type is incompatible? Could potentially still keep the original (infallible) version and panic if the new type is invalid. Doing that means people upgrading to arrow-57 won't necessarily realize there's a potential problem until their code starts crashing, which I don't love, but I suspect some people would rather deal with a potential panic than have to rewrite all their `with_data_type` calls? (alternatively -- maybe we leave the panicky infallible version in place, but deprecated with an appropriate warning?
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel closed pull request #8350: Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module
URL: https://github.com/apache/arrow-rs/pull/8350
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357600500
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
This change seems really dangerous... it will silently strip away the extension type information, which can propagate into e.g. the parquet file the column eventually gets written to, which could be very hard to fix later on when downstream readers start noticing the extension type info is unexpectedly missing.
Seems better to make this fallible, and blow up if there is an extension type for which the new data type is incompatible? Could potentially still keep the original (infallible) version and panic if the new type is invalid. Doing that means people upgrading to arrow-57 won't necessarily realize there's a potential problem until their code starts crashing, which I don't love, but I suspect some people would rather deal with a potential panic than have to rewrite all their `with_data_type` calls? Alternatively -- maybe we leave the panicky infallible version in place, but deprecated with an appropriate warning? But people with automated builds won't necessarily _see_ the warning, especially if they already have lots of warnings in their code.
If somebody _actually_ wanted to clear the extension type, it seems much better to require them to clearly state their intent by e.g. `field.without_extension_type().with_data_type(...)`
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2369622082
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
In order to support mapping strings --> types we would have to add some sort of 'registry' for types I think
IN datafusion we have `SessionState` for this kind of information, but there is nothing analogous in arrow / parquet that I know of (and I am not sure if there should be)
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357600500
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
This change seems really dangerous... it will silently strip away the extension type information, which can propagate into e.g. the parquet file the column eventually gets written to, which could be very hard to fix later on when downstream readers start noticing the extension type info is unexpectedly missing.
Seems better to make this fallible, and blow up if there is an extension type for which the new data type is incompatible?
We could potentially still keep the original (infallible) version and panic if the new type is invalid. Doing that means people upgrading to arrow-57 won't necessarily realize there's a potential problem until their code starts crashing, which I don't love, but I suspect some people would rather deal with a potential panic than have to rewrite all their `with_data_type` calls?
Alternatively -- maybe we leave the panicky infallible version in place, but deprecated with an appropriate warning? But people with automated builds won't necessarily _see_ the warning, especially if they already have lots of warnings in their code.
If somebody _actually_ wanted to clear the extension type, it seems much better to require them to clearly state their intent by e.g. `field.without_extension_type().with_data_type(...)`
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357600500
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
This change seems really dangerous... it will silently strip away the extension type information, which can propagate into e.g. the parquet file the column eventually gets written to, which could be very hard to fix later on when downstream readers start noticing the extension type info is unexpectedly missing.
Seems better to make this fallible, and blow up if there is an extension type for which the new data type is incompatible? Could potentially still keep the original (infallible) version and panic if the new type is invalid. Doing that means people upgrading to arrow-57 won't necessarily realize there's a potential problem until their code starts crashing, which I don't love, but I suspect some people would rather deal with a potential panic than have to rewrite all their `with_data_type` calls? Alternatively -- maybe we leave the panicky infallible version in place, but deprecated with an appropriate warning?
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#issuecomment-3305512944
> Adds two `Field` methods:
>
> * `Field::clear_extension_type`
>
> * `Field::without_extension_type`
>
>
> These methods are used when changing data types via `Field::set_data_type` and `Field::with_data_type`.
I don't think we need both? The current implementation of `Field::with_data_type` just delegates to `Field::set_data_type`?
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2364915885
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
Hmm, that is indeed awkward that we don't have any easy way to map from a random extension name string to the actual type :(
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357600500
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
This change seems really dangerous... it will silently strip away the extension type information, which can propagate into e.g. the parquet file the column eventually gets written to, which could be very hard to fix later on when downstream readers start noticing the extension type info is unexpectedly missing.
Seems better to make this fallible, and blow up if the new data type is incompatible is incompatible with the currently-installed extension type?
We could potentially still keep the original (infallible) version and panic if the new type is invalid. Doing that means people upgrading to arrow-57 won't necessarily realize there's a potential problem until their code starts crashing, which I don't love, but I suspect some people would rather deal with a potential panic than have to rewrite all their `with_data_type` calls?
Alternatively -- maybe we leave the panicky infallible version in place, but deprecated with an appropriate warning? But people with automated builds won't necessarily _see_ the warning, especially if they already have lots of warnings in their code.
If somebody _actually_ wanted to clear the extension type, it seems much better to require them to clearly state their intent by e.g. `field.without_extension_type().with_data_type(...)`
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357600500
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
This change seems really dangerous... it will silently strip away the extension type information, which can propagate into e.g. the parquet file the column eventually gets written to, which could be very hard to fix later on when downstream readers start noticing the extension type info is unexpectedly missing.
Seems better to make this fallible, and blow up if there is an extension type for which the new data type is incompatible? Could potentially still keep the original (infallible) version and panic if the new type is invalid. Doing that means people upgrading to arrow-57 won't necessarily realize there's a potential problem until their code starts crashing, which I don't love, but I suspect some people would rather deal with a potential panic than have to rewrite all their `with_data_type` calls? Alternatively -- maybe we leave the panicky infallible version in place, but deprecated with an appropriate warning?
If somebody _actually_ wanted to clear the extension type, it seems much better to require them to clearly state their intent by e.g. `field.without_extension_type().with_data_type(...)`
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357766806
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
I agree that this is not a good solution. It would be nice to prevent invalid state via the API of `Field`, however, with `set_metadata` and `metadata_mut` it's already possible to set datatype incompatible extension type information, so either we accept it (and replace the clear calls added in this PR with a warning in the docs) or we reconsider the mutating methods of the `Field` API.
> Seems better to make this fallible (with_data_type becomes try_with_data_type), and blow up if the new data type is incompatible with the currently-installed extension type?
The problem is you can't check data type compatibility without the actual extension type.
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2353067179
##########
arrow-schema/src/field.rs:
##########
@@ -378,10 +381,14 @@ impl Field {
#[inline]
pub fn set_data_type(&mut self, data_type: DataType) {
self.data_type = data_type;
+ self.clear_extension_type();
Review Comment:
I was initially worried that this would obliterate metadata that previously had been propagated, but I couldn't find any existing usage of this function (I don't even think it's tested in arrow-rs) and so I think it's probably not a problem.
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2353095562
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
Ah, I missed this.
This will probably obliterate metadata in some places. There's a bunch of code in datafusion that looks like this:
```
let new_fields = fields
.into_iter()
.zip(new_data_types)
.map(|(f, d)| f.as_ref().clone().with_data_type(d))
.map(Arc::new)
.collect::<Vec<FieldRef>>();
```
...where `d` is often unchanged but the outer field may have contained metadata. Sometimes the data type change is compatible with an extension type (e.g., reconciling String and StringViews), sometimes it is not (e.g., if there is any code that reconciles FixedSizeBinary and Binary inputs, which there may not be).
Using a `Field` as a data type is a constant whack-a-mole of these types of issues but I think this might be safer propagating the extension metadata until downstream projects have a chance to move away from that.
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#issuecomment-3451567269
Closing this for now (https://github.com/apache/arrow-rs/pull/8597), I may get back to the `Field` extension type methods later with another approach.
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2358418397
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
I think clear documentation warnings are best because (as @mbrobbel says) you don't know what combination is valid without the actual extension type.
Perhaps we can also add some examples of "how to change datatype and still validate extension types" or something to make it easier for downstream users to verify the expected extension types are still valid after modification
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357600500
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
This change seems really dangerous... it will silently strip away the extension type information, which can propagate into e.g. the parquet file the column eventually gets written to, which could be very hard to fix later on when downstream readers start noticing the extension type info is unexpectedly missing.
Seems better to make this fallible, and blow up if the new data type is incompatible with the currently-installed extension type?
We could potentially still keep the original (infallible) version and panic if the new type is invalid. Doing that means people upgrading to arrow-57 won't necessarily realize there's a potential problem until their code starts crashing, which I don't love, but I suspect some people would rather deal with a potential panic than have to rewrite all their `with_data_type` calls?
Alternatively -- maybe we leave the panicky infallible version in place, but deprecated with an appropriate warning? But people with automated builds won't necessarily _see_ the warning, especially if they already have lots of warnings in their code.
If somebody _actually_ wanted to clear the extension type, it seems much better to require them to clearly state their intent by e.g. `field.without_extension_type().with_data_type(...)`
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357600500
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
This change seems dangerous... it will silently strip away the extension type information, which can propagate into e.g. the parquet file the column eventually gets written to, which could be very hard to fix later on once readers start noticing the extension type info is unexpectedly missing.
Seems better to make this fallible, and blow up if there is an extension type for which the new data type is incompatible? Could potentially still keep the original (infallible) version and panic if the new type is invalid. Doing that means people upgrading to arrow-57 won't necessarily realize there's a potential problem until their code starts crashing, which I don't love, but I suspect some people would rather deal with a potential panic than have to rewrite all their `with_data_type` calls? (alternatively -- maybe we leave the panicky infallible version in place, but deprecated with an appropriate warning?
##########
arrow-schema/src/field.rs:
##########
@@ -366,7 +366,10 @@ impl Field {
&self.data_type
}
- /// Set [`DataType`] of the [`Field`]
+ /// Set [`DataType`] of the [`Field`].
+ ///
+ /// This clears extension type information from this field by calling
+ /// [`Field::clear_extension_type`].
Review Comment:
```suggestion
/// Set [`DataType`] of the [`Field`], removing any extension type information in the process.
```
##########
arrow-schema/src/field.rs:
##########
@@ -378,10 +381,14 @@ impl Field {
#[inline]
pub fn set_data_type(&mut self, data_type: DataType) {
self.data_type = data_type;
+ self.clear_extension_type();
}
/// Set [`DataType`] of the [`Field`] and returns self.
///
+ /// This clears extension type information from the returned field by
+ /// calling [`Field::without_extension_type`].
Review Comment:
```suggestion
/// Returns a new field with the requested [`DataType`].
///
/// Extension type information (if any) is not propagated.
```
##########
arrow-schema/src/field.rs:
##########
@@ -378,10 +381,14 @@ impl Field {
#[inline]
pub fn set_data_type(&mut self, data_type: DataType) {
self.data_type = data_type;
+ self.clear_extension_type();
Review Comment:
If nobody actually uses `set_data_type`, can we deprecate and remove it?
It's kind of a weird API surface, especially with the new extension type semantics
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357600500
##########
arrow-schema/src/field.rs:
##########
@@ -391,6 +398,49 @@ impl Field {
/// ```
pub fn with_data_type(mut self, data_type: DataType) -> Self {
self.set_data_type(data_type);
+ self.without_extension_type()
Review Comment:
This change seems really dangerous... it will silently strip away the extension type information, which can propagate into e.g. the parquet file the column eventually gets written to, which could be very hard to fix later on when downstream readers start noticing the extension type info is unexpectedly missing.
Seems better to make this fallible, and blow up if there is an extension type for which the new data type is incompatible? Could potentially still keep the original (infallible) version and panic if the new type is invalid. Doing that means people upgrading to arrow-57 won't necessarily realize there's a potential problem until their code starts crashing, which I don't love, but I suspect some people would rather deal with a potential panic than have to rewrite all their `with_data_type` calls? (alternatively -- maybe we leave the panicky infallible version in place, but deprecated with an appropriate warning?
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2357591422
##########
arrow-schema/src/field.rs:
##########
@@ -366,7 +366,10 @@ impl Field {
&self.data_type
}
- /// Set [`DataType`] of the [`Field`]
+ /// Set [`DataType`] of the [`Field`].
+ ///
+ /// This clears extension type information from this field by calling
+ /// [`Field::clear_extension_type`].
Review Comment:
```suggestion
/// Set [`DataType`] of the [`Field`], clearing any extension type information in the process.
```
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#discussion_r2358419797
##########
arrow-schema/src/extension/mod.rs:
##########
@@ -16,8 +16,6 @@
// under the License.
//! Extension types.
-//!
-//! <div class="warning">This module is experimental. There might be breaking changes between minor releases.</div>
Review Comment:
🎉
--
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] Add `Field` methods to clear extension types, remove `experimental` warning from `extension` module [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8350:
URL: https://github.com/apache/arrow-rs/pull/8350#issuecomment-3293574819
THANK YOU!
--
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