[SPARK-30166][SQL] Eliminate compilation warnings in JSONOptions

[MaxGekk]

What changes were proposed in this pull request?

In the PR, I propose to replace setJacksonOptions() in JSONOptions by buildJsonFactory() which builds JsonFactory using JsonFactoryBuilder. This allows to avoid using deprecated feature configurations from JsonParser.Feature.

Why are the changes needed?

• The changes eliminate the following compilation warnings in sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JSONOptions.scala:

 Warning:Warning:line (137)Java enum ALLOW_NUMERIC_LEADING_ZEROS in Java enum Feature is deprecated: see corresponding Javadoc for more information.
factory.configure(JsonParser.Feature.ALLOW_NUMERIC_LEADING_ZEROS, allowNumericLeadingZeros)
Warning:Warning:line (138)Java enum ALLOW_NON_NUMERIC_NUMBERS in Java enum Feature is deprecated: see corresponding Javadoc for more information.
factory.configure(JsonParser.Feature.ALLOW_NON_NUMERIC_NUMBERS, allowNonNumericNumbers)
Warning:Warning:line (139)Java enum ALLOW_BACKSLASH_ESCAPING_ANY_CHARACTER in Java enum Feature is deprecated: see corresponding Javadoc for more information.
factory.configure(JsonParser.Feature.ALLOW_BACKSLASH_ESCAPING_ANY_CHARACTER,
Warning:Warning:line (141)Java enum ALLOW_UNQUOTED_CONTROL_CHARS in Java enum Feature is deprecated: see corresponding Javadoc for more information.
factory.configure(JsonParser.Feature.ALLOW_UNQUOTED_CONTROL_CHARS, allowUnquotedControlChars)

• This put together building JsonFactory and set options from JSONOptions. So, we will not forget to call setJacksonOptions in the future.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By JsonSuite, JsonFunctionsSuite, JsonExpressionsSuite.

[HyukjinKwon]

Seems fine if tests pass.

[srowen]

This kind of change is fine in general. My worry is just how well it might work if Jackson is downgraded, but, only so much we can worry about this. If it passes vs current profiles, OK.

[SparkQA]

Test build #114992 has finished for PR 26797 at commit 009103a.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[MaxGekk]

My worry is just how well it might work if Jackson is downgraded, but, only so much we can worry about this.

This won't work on previous Jackson versions. We have to revert the changes if we need to downgrade Jackson.

We could point out this PR in #26131 to make the downgrade easier.

[HyukjinKwon]

Hm, that's actually a good point because Jackson version is often downgraded.

[HyukjinKwon]

Let me leave it to @srowen.

[srowen]

I've kind of lost track of whether deploying on Hadoop 2.7 means it downgrades at runtime somehow, because Hadoop 2.x uses an older version. It evidently doesn't cause test failures at least. I really don't have a specific case for you why it won't work, but just know this has been troublesome in the past and so I just haven't touched Jackson if not necessary. @steveloughran , eh, any thoughts on the state of Jackson in Hadoop?

[MaxGekk]

@srowen If such problem exists, maybe it makes sense to shade Jackson 2.10 and use the shaded version in JSON datasource?

[srowen]

It probably does make sense to shade, to isolate whatever Spark uses Jackson for a little further. I might not make this change right now otherwise, by itself, purely out of my general fear of these issues. That said ... I feel like we have already made some Jackson changes that probably break compatibility with 2.6.

[dongjoon-hyun]

Retest this please.

[SparkQA]

Test build #115265 has finished for PR 26797 at commit 009103a.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[MaxGekk]

I feel like we have already made some Jackson changes that probably break compatibility with 2.6.

Probably. I don't know is there any test which checks compatibility with Jackson v2.6. If not, rejecting PRs like this will not safe us from breaking compatibility.

Also it would nice to understand why do we really need the compatibility. As far as I know Jackson v3 is going to drop the deprecated classes and functions replaced by this PR. Are we going to stuck on v2.x forever?

[srowen]

I'm OK with merging it as I don't think this by itself breaks anything additional. If we find it doesn't work with Hadoop 2.x or something, well, we can evaluate what has to give -- just how it is, or we need to revert more than this. that said I am pretty confident we need to be able to update Jackson for security reasons, well beyond 2.6.x.

[srowen]

Merged to master

[steveloughran]

@steveloughran , eh, any thoughts on the state of Jackson in Hadoop?

a mess. We need to strip out use of jackson marshalling as that's where the security issues always surface; its not been great for backwards compat. Not sure what branch-2 latest is up to.

we are planning a 2.10 release (last of the branch-2's)...now is time to get your jackson version needs catered for