[SPARK-48148][FOLLOWUP] Fix JSON parser feature flag. #49018
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
@sadikovi @HyukjinKwon please take a look. Thanks! |
sadikovi
approved these changes
Dec 1, 2024
HyukjinKwon
approved these changes
Dec 1, 2024
Member
|
Merged to master. |
dongjoon-hyun
pushed a commit
that referenced
this pull request
Jan 10, 2025
…sk` in `JacksonParser` ### What changes were proposed in this pull request? In #49018, the restoration logic for feature flags was fixed using the `setFeatureMask` method. However, the `setFeatureMask` method has been deprecated since Jackson 2.7, so this pr reimplements the relevant logic using `overrideStdFeatures`. ### Why are the changes needed? Clean up the use of deprecated APIs. https://github.com/FasterXML/jackson-core/blob/0d2b0f39200d466f49f1abb06d9027053d41483d/src/main/java/com/fasterxml/jackson/core/JsonParser.java#L999-L1035 ``` /** * Bulk set method for (re)setting states of all standard {link Feature}s * * param mask Bit mask that defines set of features to enable * * return This parser, to allow call chaining * * since 2.3 * deprecated Since 2.7, use {link #overrideStdFeatures(int, int)} instead */ Deprecated public JsonParser setFeatureMask(int mask) { _features = mask; return this; } /** * Bulk set method for (re)setting states of features specified by <code>mask</code>. * Functionally equivalent to *<code> * int oldState = getFeatureMask(); * int newState = (oldState & ~mask) | (values & mask); * setFeatureMask(newState); *</code> * but preferred as this lets caller more efficiently specify actual changes made. * * param values Bit mask of set/clear state for features to change * param mask Bit mask of features to change * * return This parser, to allow call chaining * * since 2.6 */ public JsonParser overrideStdFeatures(int values, int mask) { int newState = (_features & ~mask) | (values & mask); return setFeatureMask(newState); } ``` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? - Pass GitHub Actions - Specialized tests have already been added in #49018: "feature mask should remain unchanged" in `JacksonParserSuite`. ### Was this patch authored or co-authored using generative AI tooling? No Closes #49434 from LuciferYang/setFeatureMask. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
#46408 attempts to set the feature flag
INCLUDE_SOURCE_IN_LOCATIONin the JSON parser and reverts the flag to the original value. The reverting code is incorrect and accidentally sets theAUTO_CLOSE_SOURCEfeature to false. The reason is thatoverrideStdFeatures(value, mask)sets the feature flags selected bymasktovalue.originalMaskis a value of 0/1. When it is 1, it selectsAUTO_CLOSE_SOURCE, whose ordinal is 0 (reference). The old code doesn't revertINCLUDE_SOURCE_IN_LOCATIONto the original value either. As a result, when the JSON parser is closed, the underlying input stream is not closed, which can lead to memory leak.Why are the changes needed?
Perform the originally intended feature, and avoid memory leak.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
New unit test. It would fail without the change in the PR.
Was this patch authored or co-authored using generative AI tooling?
No.