-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-28978:Remove Json-Path from Hive #5831
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
base: master
Are you sure you want to change the base?
Conversation
Reviewing. I verified
|
I verified this patch upgrades all places using
|
Nashorn uses
I wonder if we are allowed to include Nashorn or if we can remove it. Anyway, I'm asking it first. |
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.
Why should we do this in Hive and not in Calcite which is the dependency that brings in transitively json-path?
pom.xml
Outdated
@@ -1298,6 +1299,18 @@ | |||
<artifactId>json-path</artifactId> | |||
<version>${json-path.version}</version> |
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.
Instead of upgrading json-smart, I was wondering if we could drop json-path completely? Have you explored this option?
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.
The dep comes from Calcite and in the context of Hive I get the impression that we don't use that code path so maybe we can get rid of this dep especially since json-path does not seem to be actively maintained.
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.
I will try it out on local and see how it goes.
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.
The build will probably pass but try also to run some tests to see that they don't fail.
Have tried removing json-path completely from hive. |
59f113c
to
e3395c9
Compare
@devaspatikrishnatri By removing the json-path, I was thinking actually excluding it from calcite and not having it at all in the Hive dependency tree. |
@zabetak Let me try that. I was under wrong impression. |
Dep Tree. |
@zabetak I do not think we can completely drop json-path , there are multiple test failures like this We can remove all direct references from hive and keep it coming from caclite , and maintain this dependency there itself. |
@zabetak Any suggestions on which of the above two is the preferred approach ? |
@devaspatikrishnatri The way it seems is that we have a problem in Calcite and by pinning the dependency in Hive we are making it a Hive problem as well. Probably the best would be to let Calcite it deal with its transitive dependencies. Anyways pinning version is in general risky especially when we start touching transitively L+3 (calcite->json-path->json-smart) dependencies. Summing up I am leaning towards removing all direct references of json-path from hive. |
Got it . Will do the needful. |
|
@devaspatikrishnatri is there a plan to fix failed tests for this change. |
What changes were proposed in this pull request?
json-smart comes as a part of json-path , and the latest release of json-path was 2.9.0 in Jan 2024 , which brings in json-smart 2.5.0.This drops json-path explicitly from hive as it is not used.
Why are the changes needed?
fix CVEs
Does this PR introduce any user-facing change?
No
How was this patch tested?
Built locally, relying on pre-commits.