Remove dict_id from schema and make it an IPC concern only

[brancz]

Which issue does this PR close?

• Closes Relax dict_id equality in field merging #6356
• closes Remove dict_id from arrow_schema::field::Field and make dictionary IDs an internal implementation detail of flight encoding/decoding #5981
• closes Dictionary IDs Arrow IPC #1206

Rationale for this change

See the above issues. And this is a follow up to

• File writer preserve dict bug #6711
• Add deprecation warnings for everything related to dict_id #6873.

What changes are included in this PR?

Removal of dict_id from the canonical Schema's Field and the resulting repercussions of that.

Are there any user-facing changes?

Yes, the previously declared deprecations and a few more resulting changes that we didn't foresee previously.

The ones we did not previously foresee involve those places where we now need to pipe through an IPC schema where we previously only had the regular Schema (which used to be enough since that carried the dict_id):

• flight_data_to_arrow_batch (arrow-flight)
• arrow_data_from_flight_data (arrow-flight; sql)
• read_record_batch (arrow-ipc)
• read_dictionary (arrow-ipc)
• FileDecoder::new (arrow-ipc)

I think these don't have stability guarantees but for the sake of completeness:

• field_to_json (arrow-integration-test)
• record_batch_from_json (arrow-integration-test)
• array_from_json (arrow-integration-test)
• dictionary_array_from_json (arrow-integration-test)

@tustvold @alamb @thinkharderdev

[tustvold]

It seems unfortunate to have methods requiring multiple different types of schema passed to them.

I'm afraid I don't have the capacity at the moment to disentangle what this PR is actually changing, the flight changes in particular I think will lead to a breaking change that hasn't been communicated via a prior deprecation.

I'm going to have to leave this one to other maintainers to look into, I no longer have the necessary context to follow what the IPC APIs are doing anymore.

[brancz]

In all cases we have the option to remove the “regular” schema, but it would have to mean that we construct it wherever we need the arrow-schema schema, which we can do but it’s a schema ref almost everywhere so it seemed like that was intentional for performance or lifetime reasons. I can definitely make that change if that’s the path we want to try there’s a danger though that it is perf sensitive and then we need to end up breaking the api twice.

[adriangb]

@brancz I think it would help if we could break this PR up into smaller pieces. I came here looking to change Field::try_merge to drop the dictionary id checks. Is there any way we can disentangle these smaller changes from the larger IPC changes and such?

[brancz]

I guess we theoretically could, but that seems inconsistent with the rest of the codebase and it could cause unexpected failure and behavior without the remaining changes of this PR.

@alamb how do you propose we continue with this? I'm happy to rebase and fix the remaining things, if we can agree on the general trajectory of continuing with this strategy.

[alamb]

I guess we theoretically could, but that seems inconsistent with the rest of the codebase and it could cause unexpected failure and behavior without the remaining changes of this PR.

@alamb how do you propose we continue with this? I'm happy to rebase and fix the remaining things, if we can agree on the general trajectory of continuing with this strategy.

I like @adriangb 's suggestion to try to break this PR up into smaller changes as it will be easier to review. Specifically I think it will be easier to achieve what @tustvold describes as:

disentangle what this PR is actually changing

I am finding this PR hard to review because I feel it is hard to understand the implications of the change (on compatibility, performance, etc). Smaller PRs might make that easier

However, there might also be no way to break it up smaller . Some thoughts I had:

1. changes to flight in one PR
2. changes to the integration test (to ignore / stop using the ipc fields)
3. changes to the reader/writer
4. Then remove the fields

[brancz]

I split out just the IPC layer changes in #7929. That would have to be merged first in order to do the remaining changes that rely on the IPC layer no longer using the dict_id field.

[brancz]

I was able to split this PR into 5 separate PRs so I'm closing this one.

[alamb]

I was able to split this PR into 5 separate PRs so I'm closing this one.

Thank you very much for doing so 🤗