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.

Sign up for GitHub

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

Jump to bottom

jsonpath_lib_polars_vendor should not enable the serde_json's preserve_order feature by default #20208

Open
2 tasks done
photino opened this issue Dec 7, 2024 · 9 comments · May be fixed by #20225
Open
2 tasks done

jsonpath_lib_polars_vendor should not enable the serde_json's preserve_order feature by default #20208

photino opened this issue Dec 7, 2024 · 9 comments · May be fixed by #20225
Labels
bug Something isn't working needs triage Awaiting prioritization by a maintainer rust Related to Rust Polars

Comments

@photino
Copy link

photino commented Dec 7, 2024 •
edited
Loading

Checks

  • I have checked that this issue has not already been reported.
  • I have confirmed this bug exists on the latest version of Polars.

Reproducible example

[dependencies.polars]
version = "0.43.1"
optional = true
default-features = false
features = [
    "csv",
    "json",
    "lazy",
    "performant",
    "sql",
]

Log output

No response

Issue description

The "preserve_order" feature will make std::mem::size_of::<serde_json::Value>() == 72 rather than 32. It should not be enabled by default.

[dependencies.serde_json]
features = ["preserve_order"]
version = "1.0"

Expected behavior

Remove the "preserve_order" feature of serde_json.

Installed versions

"csv",
"json",
"lazy",
"performant",
"sql"
@photino photino added bug Something isn't working needs triage Awaiting prioritization by a maintainer rust Related to Rust Polars labels Dec 7, 2024
@ritchie46
Copy link
Member

Can we be certain it doesn't have changes to result?

@photino
Copy link
Author

photino commented Dec 8, 2024

Operations on JSON object should not rely on the order of keys.

@ritchie46
Copy link
Member

Sure, that sounds all good theoretical, I just want to make sure we don't have a breaking change.

@ritchie46
Copy link
Member

Maybe you can propose a PR and see how are test suite runs?

@photino
Copy link
Author

photino commented Dec 8, 2024

Where does the source of jsonpath_lib_polars_vendor locate? It seems that the docs link to https://github.com/freestrings/jsonpath

@ritchie46
Copy link
Member

https://github.com/ritchie46/jsonpath/tree/improve_compiled

@photino photino mentioned this issue Dec 8, 2024
Merged
@photino
Copy link
Author

photino commented Dec 9, 2024

https://github.com/pola-rs/polars/blob/main/crates/polars-ops/Cargo.toml#L43

Should we change this dependency to the git path to see how the test suite runs?

@ritchie46
Copy link
Member

ritchie46 commented Dec 9, 2024 •
edited
Loading

Yes, and if all goes well, I can republish. (It can probably also use a dependency update)

@coastalwhite
Copy link
Collaborator

coastalwhite commented Dec 9, 2024 •
edited
Loading

To be honest, I am a bit skeptical of this change. We currently serialize structs as JSON objects. If I understand correctly, not preserving order would mean that they potentially come back in a different order. And this is UB, so not deterministic.

If this is wanted, which I can totally imagine. It would most likely involve more changes than just changing the enabled features.

photino added a commit to photino/polars that referenced this issue Dec 9, 2024
@photino
Update the jsonpath_lib dependency
9e5f102 
Fixes pola-rs#20208
@photino photino linked a pull request Dec 9, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working needs triage Awaiting prioritization by a maintainer rust Related to Rust Polars
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Update the jsonpath_lib dependency photino/polars
3 participants
@ritchie46 @photino @coastalwhite