[SPARK-27512][SQL] Avoid to replace ',' in CSV's decimal type inference for backward compatibility

[HyukjinKwon]

What changes were proposed in this pull request?

The code below currently infers as decimal but previously it was inferred as string.

In branch-2.4, type inference path for decimal and parsing data are different.

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala

Line 153 in 2a83431

val value = new BigDecimal(datum.replaceAll(",", ""))

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala

Line 125 in c284c4e

val bigDecimal = new BigDecimal(field)

So the code below:

scala> spark.read.option("delimiter", "|").option("inferSchema", "true").csv(Seq("1,2").toDS).printSchema()

produced string as its type.

root
 |-- _c0: string (nullable = true)

In the current master, it now infers decimal as below:

root
 |-- _c0: decimal(2,0) (nullable = true)

It happened after #22979 because, now after this PR, we only have one way to parse decimal:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala

Line 92 in 7a83d71

(s: String) => new java.math.BigDecimal(s.replaceAll(",", ""))

After the fix:

root
 |-- _c0: string (nullable = true)

This PR proposes to restore the previous behaviour back in CSVInferSchema.

How was this patch tested?

Manually tested and unit tests were added.

[HyukjinKwon]

Honestly, I think that it's also okay to drop this weird previous behaviour ... but just decided to follow the intention of keeping backward compatibility since it's simple, for now.

I think we should drop this replace(",") behaviour at all soon.

[HyukjinKwon]

cc @MaxGekk and @cloud-fan

[MaxGekk]

What about to support prefersDecimal option for CSV as we have for JSON:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JSONOptions.scala

Lines 54 to 55 in ef2d63b

	val prefersDecimal =
	parameters.get("prefersDecimal").map(_.toBoolean).getOrElse(false)

[HyukjinKwon]

Makes sense but I think that doesn't fully address the current issue - some people could still complain about changing this behaviour.

IIUC, prefersDecimal is a switch to decide if we infer decimal or not. In this issue, looks like users still want the decimal inference but want to avoid one case containing ,.

Let's consider that option later when we drop this behaviour later.

[cloud-fan]

Sorry I may not know all the context, but doesn't 1,0 mean 2 int columns?

[MaxGekk]

The field separator here is | not ,. So 1,0 cannot be considered as separate columns.

[SparkQA]

Test build #104804 has finished for PR 24437 at commit b9b76d5.

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

[MaxGekk]

It looks slightly ugly that we handle special case of Locale.US for JSON inside of ExprUtils.getDecimalParser but for CSV outside of it. Maybe we unify the implementation, and handle the special cases outside of generic implementation ExprUtils.getDecimalParser (or process both special cases inside of it)?

[HyukjinKwon]

Yes.. we gotta do that I think .. Let's do that later when we drop the replacement thing entirely. I realised that CSV inference path alone handles it without replacement.

[MaxGekk]

ExprUtils.getDecimalParser is used in UnivocityParser:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/UnivocityParser.scala

Line 157 in ef2d63b

Decimal(decimalParser(datum), dt.precision, dt.scale)

. Do we need to handle the backward compatibility case there as well?

[HyukjinKwon]

Nope, for parsing itself somehow replacement was being done before and it looks being handled fine

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala

Line 92 in 7a83d71

(s: String) => new java.math.BigDecimal(s.replaceAll(",", ""))

[HyukjinKwon]

Let me get this in if you guys don't mind. This just literally restores previous behaviour as was.

[HyukjinKwon]

Thanks, @MaxGekk and @cloud-fan. Merged to master.