Skip to content
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

Remove dict_id from arrow_schema::field::Field and make dictionary IDs an internal implementation detail of flight encoding/decoding #5981

Open
thinkharderdev opened this issue Jun 29, 2024 · 4 comments
Labels
enhancement Any new improvement worthy of a entry in the changelog

Comments

@thinkharderdev
Copy link
Contributor

thinkharderdev commented Jun 29, 2024

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

Currently the dict_id field is only used for the purposes of arrow flight encoding/decoding so dictionaries can be mapped to there associated fields.

This is annoying and error-prone as the user is left the responsibility of assigning these dictionary IDs and ensuring that they are unique.

#5971 adds the option to auto-assign dictionary IDs during arrow flight encoding. This can be enabled by setting the preserve_dict_id option in IpcWriteOptions to false (current default is true

Describe the solution you'd like

This can be done in stages but ultimately would like to

  1. Make preserve_dict_id default to false
  2. Remove the preserve_dict_id option altogether
  3. Remove the dict_id field from arrow_schema::schema::Field entirely as it no longer has any purpose

Describe alternatives you've considered

We can leave this is as a configurable option and either only do 1 above or we can leave auto-assigning of dictionary IDs as an opt-in feature

Additional context

@thinkharderdev thinkharderdev added the enhancement Any new improvement worthy of a entry in the changelog label Jun 29, 2024
@alamb
Copy link
Contributor

alamb commented Jul 1, 2024

I think this would be a nice thing to do -- we have talked about it for a long time.

I think the dict_id field in the schema is left over from very very early versions of arrow-rs

@brancz
Copy link
Contributor

brancz commented Sep 6, 2024

Now that dict IDs are automatically assigned, how about we create a new type eg. FlightSchema and FlightField in the flight crate that are created in prepare_schema_for_flight and wrap the canonical Schema and Field? Then we can remove dict_id from Field.

@thinkharderdev
Copy link
Contributor Author

Now that dict IDs are automatically assigned, how about we create a new type eg. FlightSchema and FlightField in the flight crate that are created in prepare_schema_for_flight and wrap the canonical Schema and Field? Then we can remove dict_id from Field.

Seems reasonable. We still need to do step 1 and 2 though. I realize now that we missed the 53.0.0 release so we can't do that until December and the next major release since it is a breaking change

@brancz
Copy link
Contributor

brancz commented Sep 6, 2024

Now that dict IDs are automatically assigned, how about we create a new type eg. FlightSchema and FlightField in the flight crate that are created in prepare_schema_for_flight and wrap the canonical Schema and Field? Then we can remove dict_id from Field.

After trying to apply this change I realize it's actually entirely useless since like you said, after 1 and 2 there won't be any use for dict_id anymore. Since the dict ID is just set in the prepare_schema_for_flight function on the dict tracker directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

No branches or pull requests

3 participants