-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Upgrade simdjson lib to 3.7.0 (from 3.2.0) #9041
Conversation
✅ Deploy Preview for meta-velox canceled.
|
@@ -31,7 +31,7 @@ by Velox. See details on bundling below. | |||
| xsimd | 10.0.0 | Yes | | |||
| re2 | 2021-04-01 | Yes | | |||
| fmt | 10.1.1 | Yes | | |||
| simdjson | 3.2.0 | Yes | | |||
| simdjson | 3.7.0 | Yes | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually REQUIRE this version (i see the version set in cml.txt but I mean in the C++? Or is it just a nice to have so we bump the version in resolve_dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @assignUser, thanks for your review!
I posted some background message in PR description. As explained, we would like to use a simdjson API which is available since 3.7.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then this change makes sense. Just wanted to make sure we are not raising the minimum version needlessly. 👍
@PHILO-HE We have some dependencies on simdjson internally, I need to make sure that these changes wont affect them. Please give me some time to look into this. |
@PHILO-HE So internally Meta still uses 3.1.8 (which is close to 3.2.0 but not exactly it). We will also need to upgrade our internal dependencies to 3.7.0 and then validate that this doesnt break anything or has any performance regressions. I will require some time for this. |
@kgpai, thanks so much for your time! |
3fd1e23
to
2b1875a
Compare
Hi @kgpai, do you have any update? Thanks! |
@PHILO-HE I'm upgrading simdjson internally to 3.8.0. Would it be possible to update to the same version here? |
Superseeded by #9456 |
Recently, we are proposing a patch to support a Spark JSON function based on simdjson lib. It requires to use json path API which is only available since 3.7.0. So we are trying to upgrade the lib.
Background:
Optimize get_json_object Spark function using simdjson #5179 (comment) (3.7.0 is enough)
Add full support for JSONPath simdjson/simdjson#2070