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 indexmap #1961

Closed
wants to merge 1 commit into from
Closed

Remove indexmap #1961

wants to merge 1 commit into from

Conversation

nevi-me
Copy link
Contributor

@nevi-me nevi-me commented Jun 29, 2022

Which issue does this PR close?

Closes #1882.

Rationale for this change

See #1882

What changes are included in this PR?

Removes the indexmap dependency to evaluate the impact on tests

Are there any user-facing changes?

Yes, arrow::json::reader::ReaderBuilder::with_format_strings used to incorrectly take an IndexMap aliased as HashMap, it and other related decoder options now take a HashMap


Note that there are other crates that depend on indexmap.

These are:

  • clap - integration-testing, in parquet as an optional feature for binaries
  • h2 - arrow-flight (dependency of tonic)
  • petgraph - arrow-flight arrow-flight (dependency of tonic-build)
  • tower - arrow-flight (dependency of tonic)

So if we can find a suitable resolution to the JSON writer, we could remove indexmap from the arrow crate.

I've also noticed that our write behaviour changes if we don't preserve_order on serde-json, which could be a blocker.

@github-actions github-actions bot added arrow Changes to the arrow crate parquet Changes to the parquet crate labels Jun 29, 2022
@nevi-me
Copy link
Contributor Author

nevi-me commented Jun 29, 2022

@alamb @jhorstmann @tustvold please see comments on the PR.
I haven't looked at the json writer failures, but I know that they depend on serde_json's preserve_order feature. So there might not be much of an alternative there.

One approach could be to isolate the json functionality into its own feature, and perhaps even go as far as splitting the read and write features to be under the json feature. It would at least enable us to remove indexmap if one doesn't need json_write

@nevi-me nevi-me closed this Jul 31, 2022
@nevi-me nevi-me deleted the remove-indexmap branch July 31, 2022 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove indexmap dependency
1 participant