You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "brancz (via GitHub)" <gi...@apache.org> on 2024/12/12 13:27:20 UTC
[PR] Add deprecation warnings for everything related to `dict_id` [arrow-rs]
brancz opened a new pull request, #6873:
URL: https://github.com/apache/arrow-rs/pull/6873
# Which issue does this PR close?
Related to https://github.com/apache/arrow-rs/issues/5981 and https://github.com/apache/arrow-rs/issues/1206
# Rationale for this change
With https://github.com/apache/arrow-rs/pull/6788 not preserving the dictionary ID is now the default, meaning we can finally remove preserving dictionary IDs, and more generally get rid of them in the `Field` struct. This adds the deprecation warnings that this will happen in the future.
# What changes are included in this PR?
All actual code changes have already landed, this just adds the deprecation warnings so we can do the breaking changes in the future.
# Are there any user-facing changes?
None yet, this is preparation for future breaking changes.
@alamb @tustvold
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "thinkharderdev (via GitHub)" <gi...@apache.org>.
thinkharderdev commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2543132904
> Ok so is the thesis that dictionary ID are just an implementation detail of the IPC format
Been checked out on this one for a bit but IIRC this was the original insight behind all of this. Browsing around through other arrow implementations in other languages they don't seem to expose the dictionary ID at all in the schema and leave it as in internal detail of IPC serde. If people are using the dictionary ID it's probably for something that should properly be done with kv metadata instead.
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2543116014
I think the existing tests that are currently relying on `allow(deprecated)` would be a good starting point
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "brancz (via GitHub)" <gi...@apache.org>.
brancz commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2543982240
Once merged I’ll start drafting changes to add the optimizations discussed as well as the dice_id removal which I’ll continually rebase over the next months and then we can land them for the March release.
Thank you very much for the productive collaboration everyone!
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2546444781
Well I already found a potential issue in DataFusion, see https://github.com/apache/datafusion/pull/13663#discussion_r1887364957
We can wait to see how that conversation is resolved
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#discussion_r1882194333
##########
arrow-ipc/src/writer.rs:
##########
@@ -65,6 +65,10 @@ pub struct IpcWriteOptions {
/// schema or generate unique dictionary IDs internally during encoding.
///
/// Defaults to `false`
+ #[deprecated(
Review Comment:
💯
##########
arrow-schema/src/field.rs:
##########
@@ -38,6 +38,10 @@ pub struct Field {
name: String,
data_type: DataType,
nullable: bool,
+ #[deprecated(
+ since = "54.0.0",
+ note = "The ability to preserve dictionary IDs will be removed. With it, all fields related to it."
+ )]
dict_id: i64,
dict_is_ordered: bool,
Review Comment:
I wonder if we should deprecate dict_is_ordered as well 🤔
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2543108939
> How would I write an IPC dictionary with an ID of 32 without using Field::dict_id? As written the deprecation notice basically reads, "we've removed this functionality, if you need it 🤷♂️", I think we need to actually provide a viable alternative. This doesn't need to be using Field::dict_id, and could be some setup using DictionaryTracker, but we do need to document whatever it is. Otherwise people lack a clear migration path.
I can help with this -- is there some test / example of what assigning dict_ids in the IPC means (aka some example of the use that I can use an example to craft the API?)
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2546349365
🚀 here we go
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2541951317
How would I write an IPC dictionary with an ID of 32 without using Field::dict_id? As written the deprecation notice basically reads, "we've removed this functionality, if you need it :man_shrugging:", I think we need to actually provide a viable alternative
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "brancz (via GitHub)" <gi...@apache.org>.
brancz commented on code in PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#discussion_r1883593446
##########
arrow-schema/src/field.rs:
##########
@@ -38,6 +38,10 @@ pub struct Field {
name: String,
data_type: DataType,
nullable: bool,
+ #[deprecated(
+ since = "54.0.0",
+ note = "The ability to preserve dictionary IDs will be removed. With it, all fields related to it."
+ )]
dict_id: i64,
dict_is_ordered: bool,
Review Comment:
I think that one is a bit less clear to me. I can see how this could be used to optimize the processing of queries, and at least at the moment I can't think of a better place for this, nor does it create problems in the same way as `dict_id` does.
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "brancz (via GitHub)" <gi...@apache.org>.
brancz commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2543146149
Ok I also took the time to analyze every single allow deprecate in tests and tl;dr is there are only 2 tests that actually test the dictionary ID ending up on the wire, but as a side effect, they actually want to test something else.
<details>
<summary>Here is my full analysis:</summary>
tests in `arrow-flight/src/encode.rs`:
* `test_schema_metadata_encoded`: doesn't care about new_with_preserve_dict_id, can be changed to new
* `flight_data_from_arrow_batch` uses default already
test in `arrow-integration-test/src/schema.rs`: just marshals whatever the field is, it doesn't care about the dict_id (simply happens to use new_dict)
test in `arrow-integration-testing/src/flight_server_scenarios/integration_test.rs`: uses new_with_preserve_dict_id, can be changed to regular `new`
test in `arrow-ipc/src/convert.rs`: just tests roundtripping, doesn't care about dict_id (simply happens to use new_dict)
tests in `arrow-ipc/src/reader.rs`:
* `test_roundtrip_nested_dict_no_preserve_dict_id`: uses new_with_preserve_dict_id, can be changed to regular `new`
* `test_roundtrip_stream_nested_dict_of_map_of_dict`: doesn't care about dict_id (simply happens to use new_dict)
* `test_roundtrip_stream_dict_of_list_of_dict`: doesn't care about dict_id (simply happens to use new_dict)
* `test_roundtrip_stream_dict_of_fixed_size_list_of_dict`: doesn't care about dict_id (simply happens to use new_dict)
* `test_roundtrip_view_types_nested_dict`: doesn't care about dict_id (simply happens to use new_dict)
* `test_unaligned`: uses new_with_preserve_dict_id, can be changed to regular `new`
* `test_unaligned_throws_error_with_require_alignment`: uses new_with_preserve_dict_id, can be changed to regular `new`
* `test_same_dict_id_without_preserve`: uses new_with_preserve_dict_id, can be changed to regular `new`, but also uses `new_dict`
* `test_read_ree_dict_record_batches_from_buffer`: doesn't care about `dict_id` and IpcWriteOptions can be changed to default
* `track_union_nested_dict`, `track_struct_nested_dict`: actually tests dict_id to be set on the wire (but not sure why it doesn't need to for the purpose of the test)
* `test_dictionary_ordered`: uses `new_dict` but doesn't care about its value
tests in `arrow-schema/src/field.rs`:
* need to look into uses of `test_new_dict_with_string` and `schema_field_with_dict_id`, `person_schema`
* `test_fields_with_dict_id`, `test_field_comparison_case`, `schema_field_accessors`: specifically tests the existance of the functions/fields we want to remove (not the functionality)
tests in `parquet/src/arrow/arrow_writer/mod.rs`:
* `arrow_writer_string_dictionary`, `arrow_writer_primitive_dictionary`, `arrow_writer_string_dictionary_unsigned_index`: just tests roundtripping, doesn't care about dict_id (simply happens to use new_dict)
tests in `parquet/src/arrow/schema/mod.rs`:
* `test_arrow_schema_roundtrip`: doesn't care about new_dict, simply calls it.
</details>
Originally coming from Arrow's implementation in Go I can also attest that this is the only library around that I've found to be exposing this and not just making it a detail of the wire format.
After going through this in detail, I conclude these deprecations are safe to do, and we should follow up with testing dictionary equality (in the now default mode) to actually set dictionary IDs equal for equal dictionaries, however, that doesn't influence the APIs, so in my opinion we can go ahead with this as is.
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2543123550
Ok so is the thesis that dictionary ID are just an implementation detail of the IPC format, if so then I guess this is fine, and we can effectively use the deprecation as a scream test. I'm a little wary we are removing functionality here, but I'm also very out of the loop w.r.t the dictionary tracker changes
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "brancz (via GitHub)" <gi...@apache.org>.
brancz commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2541925236
The problem is that it requires deprecations one way or another. If the deprecations aren't here then they're about the dictionary tracker to allow that functionality. I agree it's a dilemma.
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "brancz (via GitHub)" <gi...@apache.org>.
brancz commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2543118067
> I think the existing tests that are currently relying on allow(deprecated) would be a good starting point
I think we need to be a bit more specific, because how does this make sense for any of the `new_dict` calls? Practically none of the instances where `new_dict` is called is the dictionary ID actually relevant but it just happens to be part of the function signature. If I'm understanding you right though, you mean all the tests where the dictionary IDs value actually matters, right?
Regarding still having another ability to set dict IDs: We can introduce a new option on `IpcWriteOptions` where the DictionaryTracker can be pre-set. And then we allow a dictionary tracker to be instantiated with a pre-populated list of dict IDs. Then a user could pre-populate the IDs, so we would have a second way of setting dict IDs that will still exist when the field doesn't exist anymore. I'll put it together and we can discuss more (really want to make this work because `dict_id` being on the Field is causing a ton of problems for us regarding equality for merging schemas).
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2543899030
> Ok so is the thesis that dictionary ID are just an implementation detail of the IPC format, if so then I guess this is fine, and we can effectively use the deprecation as a scream test. I'm a little wary we are removing functionality here, but I'm also very out of the loop w.r.t the dictionary tracker changes. If Andrew is happy feel free to ignore me.
Given that this PR deprecates the APIs and doesn't remove them and no one seems to have come up with any reasonable usecase for the current dict_id encoding (despite some clearly non trivial analysis by @brancz 🙏 and @thinkharderdev ) I think this PR cuts a nice balance between:
1. a real path for removing dict_ids
2. Not causing immense downstream pain by removing an API that is needed
Let's merge this and see if anyone comes up with a motiviating usecase.
Thank you again for sticking with this @brancz
--
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 deprecation warnings for everything related to `dict_id` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6873:
URL: https://github.com/apache/arrow-rs/pull/6873#issuecomment-2543899406
I'll plan to merge this PR tomorrow unless there are other comments / concerns
cc @etseidl in case you are interested in this sort of thing
--
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