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

[thinkharderdev]

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

[alamb]

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]

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]

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]

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.

[brancz]

Once #6711 merges, we can change the default to not preserving dict IDs.

Then the next breaking release we remove the ability to set dict IDs in the first place?

[brancz]

Here is the follow up: #6788

[alamb]

See also

• Add deprecation warnings for everything related to dict_id #6873

[andygrove]

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 not entirely true. It is part of Arrow's public API and is used by downstream systems such as Comet.

[tustvold]

As noted by @andygrove on #7017, arrow-java currently also exposes the field ID as part of its schema definition.

I have double checked that the C FFI interface does not have a way to propagate this information across an FFI boundary - https://arrow.apache.org/docs/format/CDataInterface.html#the-arrowschema-structure.

Provided we preserve the ability to control the IDs used by the IPC encoder, I think this shouldn't result in a loss of functionality. Perhaps @brancz you could confirm if this is possible. The conclusion of #6873 was to conduct a scream test, I am inclined to think this has failed.

[alamb]

Copy/pasting from @andygrove on #7017 to get all the context in this ticket:

I started upgrading Comet to use the latest DataFusion/Arrow and saw that an effort is being made to remove dict_id from arrow-rs Field.

It seems odd that arrow-java has dict_id as part of its field representation, but we plan to remove it from arrow-rs.

I am not 100% sure but this may be problematic for Comet since we seem to rely on the dictionary id when passing arrays between Java and Rust. I did not write this code and am not yet familiar with it, but in one example, in Rust we have:

let mut dict_id = 0;
let fields = arrays
    .iter()
    .enumerate()
    .map(|(i, array)| match array.data_type() {
        DataType::Dictionary(_, _) => {
            let field = Field::new_dict(
                format!("c{}", i),
                array.data_type().clone(),
                true,
                dict_id,
                false,
            );
            dict_id += 1;
            field
        }
        _ => Field::new(format!("c{}", i), array.data_type().clone(), true),
    })

and then in Java:

DictionaryEncoding dictionaryEncoding = vector.getField().getDictionary();
Dictionary dictionary = dictionaryProvider.lookup(dictionaryEncoding.getId());
It will take me some time to get up to speed with how we are using this in Comet, and to determine whether it is needed or not.

[andygrove]

I looked into this more this morning and removed the uses of Field::new_dict in the PR where we are upgrading to arrow-rs 54.0.0, and there were no regressions.

Removing these uses from our main branch, which uses arrow-rs 53.3.0, causes failures in Arrow IPC, which is to be expected if I am following along correctly.

@tustvold Thanks for clarifying that this does not impact FFI. That was my main concern.