[SPARK-28137][SQL] Add Postgresql function to_number. #25963

beliefer · 2019-09-29T07:14:02Z

What changes were proposed in this pull request?

Postgresql and Oracle have the function to_number to convert a string to number.
The implement and support syntax has many different between Postgresql and Oracle. So, this PR mainly follows the implement of to_number in Postgresql.

There are some mainstream database support the syntax.
PostgreSQL:
https://www.postgresql.org/docs/12/functions-formatting.html

Oracle:
https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/TO_NUMBER.html#GUID-D4807212-AFD7-48A7-9AED-BEC3E8809866

Vertica
https://www.vertica.com/docs/10.0.x/HTML/Content/Authoring/SQLReferenceManual/Functions/Formatting/TO_NUMBER.htm?tocpath=SQL%20Reference%20Manual%7CSQL%20Functions%7CFormatting%20Functions%7C_____7

Redshift
https://docs.aws.amazon.com/redshift/latest/dg/r_TO_NUMBER.html

DB2
https://www.ibm.com/support/knowledgecenter/SSGU8G_14.1.0/com.ibm.sqls.doc/ids_sqs_1544.htm

Teradata
https://docs.teradata.com/r/kmuOwjp1zEYg98JsB8fu_A/TH2cDXBn6tala29S536nqg

Snowflake:
https://docs.snowflake.net/manuals/sql-reference/functions/to_decimal.html

Exasol
https://docs.exasol.com/sql_references/functions/alphabeticallistfunctions/to_number.htm#TO_NUMBER

Singlestore
https://docs.singlestore.com/v7.3/reference/sql-reference/numeric-functions/to-number/

Intersystems
https://docs.intersystems.com/latest/csp/docbook/DocBook.UI.Page.cls?KEY=RSQL_TONUMBER

The syntax like:

select to_number('12,454.8-', '99G999D9S');
-12454.8

Why are the changes needed?

This PR adds a new function.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

New UT.

SparkQA · 2019-09-29T07:19:09Z

Test build #111557 has finished for PR 25963 at commit 07ad71b.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ToNumber(strExpr: Expression, patternExpr: Expression)

MaxGekk

Could you uncomment examples there:

spark/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/numeric.sql

Lines 825 to 846 in 66c9dc3

    
           -- SELECT '' AS to_number_1,  to_number('-34,338,492', '99G999G999'); 
        
           -- SELECT '' AS to_number_2,  to_number('-34,338,492.654,878', '99G999G999D999G999'); 
        
           -- SELECT '' AS to_number_3,  to_number('<564646.654564>', '999999.999999PR'); 
        
           -- SELECT '' AS to_number_4,  to_number('0.00001-', '9.999999S'); 
        
           -- SELECT '' AS to_number_5,  to_number('5.01-', 'FM9.999999S'); 
        
           -- SELECT '' AS to_number_5,  to_number('5.01-', 'FM9.999999MI'); 
        
           -- SELECT '' AS to_number_7,  to_number('5 4 4 4 4 8 . 7 8', '9 9 9 9 9 9 . 9 9'); 
        
           -- SELECT '' AS to_number_8,  to_number('.01', 'FM9.99'); 
        
           -- SELECT '' AS to_number_9,  to_number('.0', '99999999.99999999'); 
        
           -- SELECT '' AS to_number_10, to_number('0', '99.99'); 
        
           -- SELECT '' AS to_number_11, to_number('.-01', 'S99.99'); 
        
           -- SELECT '' AS to_number_12, to_number('.01-', '99.99S'); 
        
           -- SELECT '' AS to_number_13, to_number(' . 0 1-', ' 9 9 . 9 9 S'); 
        
           -- SELECT '' AS to_number_14, to_number('34,50','999,99'); 
        
           -- SELECT '' AS to_number_15, to_number('123,000','999G'); 
        
           -- SELECT '' AS to_number_16, to_number('123456','999G999'); 
        
           -- SELECT '' AS to_number_17, to_number('$1234.56','L9,999.99'); 
        
           -- SELECT '' AS to_number_18, to_number('$1234.56','L99,999.99'); 
        
           -- SELECT '' AS to_number_19, to_number('$1,234.56','L99,999.99'); 
        
           -- SELECT '' AS to_number_20, to_number('1234.56','L99,999.99'); 
        
           -- SELECT '' AS to_number_21, to_number('1,234.56','L99,999.99'); 
        
           -- SELECT '' AS to_number_22, to_number('42nd', '99th');

MaxGekk · 2019-09-29T07:58:52Z

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

 }

+/**
+ * A function that convert string to numeric.


convert -> converts

OK. Thanks for your remind.

MaxGekk · 2019-09-29T08:06:34Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

+   * @group string_funcs
+   * @since 3.0.0
+   */
+  def to_number(x: Column, format: String): Column = withExpr {


Registering the to_number function should be enough. Exposing it in Scala API is not necessary.

OK. I will remove it.

dongjoon-hyun · 2019-09-29T21:40:40Z

Hi, @beliefer . Please run dev/scalastyle and fix all of the errors.

SparkQA · 2019-09-30T03:48:03Z

Test build #111591 has finished for PR 25963 at commit 7b4ed77.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-09-30T06:52:10Z

Test build #111592 has finished for PR 25963 at commit c4a170c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-09-30T10:53:16Z

Test build #111602 has finished for PR 25963 at commit a8286cf.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-10-08T07:05:02Z

Test build #111870 has finished for PR 25963 at commit 2a7202a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

beliefer · 2019-10-08T07:08:25Z

Retest this please.

SparkQA · 2019-10-08T11:01:12Z

Test build #111883 has finished for PR 25963 at commit 2a7202a.

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

beliefer · 2019-10-08T11:32:16Z

@MaxGekk Could you review this PR continuously ?

SparkQA · 2019-10-08T15:09:52Z

Test build #111889 has finished for PR 25963 at commit 0f51086.

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

beliefer · 2019-10-09T02:01:00Z

@dongjoon-hyun @wangyum Could you help me to review this PR?

MaxGekk · 2019-10-09T14:04:38Z

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

 }

+/**
+ * A function that converts string to numeric.


This comments duplicates the usage part. I don't think it is useful.

I will preserve this line and change the usage part as Convert strExprto a number based on thepatternExpr.

MaxGekk · 2019-10-09T14:05:31Z

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

+/**
+ * A function that converts string to numeric.
+ */
+// scalastyle:off line.size.limit


The code below doesn't reach the limit. This can be removed.

Thanks, I will follow your suggestion.

MaxGekk · 2019-10-09T14:11:31Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala

      Literal.create(null, IntegerType), Literal.create(null, IntegerType)), null)
  }

+  test("ToNumber") {


The usage part of the expressions says that some part of patterns can use locale:

'S': sign anchored to number (uses locale) 'L': currency symbol (uses locale) 'D': decimal point (uses locale) 'G': group separator (uses locale)

Could you test a few locales.

@MaxGekk Thanks for this review. I test the locale on Postgresql but 'L' seems not works.

select to_number('USD34234.4350', 'L99999.0000'); // 34234.435 select to_number('EUR34234.4350', 'L99999.0000'); // 34234.435 select to_number('RY34234.4350', 'L99999.0000'); // 34234.435

Although 'RY ' is not a valid locale , the result is the same as the others .

I think the description of locales is not consistent with the behavior in fact. Maybe I should remove the comment for locale as it was a bug of Postgresql.

MaxGekk · 2019-10-09T14:13:03Z

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

+      '0': digit position (will not be dropped, even if insignificant)
+      '.': decimal point (only allowed once)
+      ',': group (thousands) separator
+      'S': sign anchored to number (uses locale)


Could clarify from where it takes the locale.

MaxGekk · 2019-10-09T14:20:04Z

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

+  extends BinaryExpression with ImplicitCastInputTypes {
+
+  // scalastyle:off caselocale
+  private lazy val patternStr = patternExpr.eval().asInstanceOf[UTF8String].toUpperCase.toString


Here, you assume that patternExpr is foldable expression, correct? What happens if it is not. You can see there how to handle both cases:

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

Lines 615 to 619 in eecef75

@transient private lazy val formatter: Option[TimestampFormatter] = {

if (right.foldable) {

Option(right.eval()).map(format => TimestampFormatter(format.toString, zoneId))

} else None

}

OK. A good suggestion.

MaxGekk · 2019-10-09T14:22:50Z

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

+
+    val inputTypeCheck = super.checkInputDataTypes()
+    if(inputTypeCheck.isSuccess) {
+      if (patternStr.count(checkDecimalPointNum(_)) > 1) {


Suggested change

if (patternStr.count(checkDecimalPointNum(_)) > 1) {

if (patternStr.count(checkDecimalPointNum) > 1) {

OK. I learned it.

MaxGekk · 2019-10-09T14:25:30Z

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

+      }
+    }
+
+    var result = if (integerLen == -1 && wholeLen == -1) {


Suggested change

var result = if (integerLen == -1 && wholeLen == -1) {

val result = if (integerLen == -1 && wholeLen == -1) {

MaxGekk · 2019-10-09T14:33:52Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala

+    checkEvaluation(ToNumber(Literal("RY34234.4350"), Literal("L99999.0000")), "34234.435")
+    checkEvaluation(ToNumber(Literal("R34234.4350"), Literal("L99999.0000")), "34234.435")
+
+    ToNumber(Literal("454.3.2"), Literal("999D9D9")).checkInputDataTypes() match {


Here you check only one negative tests. Could you add a little bit more negative tests.

For example here is a test coverage report:

Here you check only one negative tests. Could you add a little bit more negative tests.

OK. I will add more.

SparkQA · 2019-10-17T13:40:57Z

Test build #112216 has finished for PR 25963 at commit 8deaabf.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ToNumber(left: Expression, right: Expression)

beliefer · 2019-10-18T01:59:10Z

@MaxGekk Could you review this PR continuously ? cc @dongjoon-hyun

MaxGekk · 2019-10-18T18:42:34Z

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

+      c == '.' || c == 'D'
+    }
+
+    def checkSignNum(c: Char): Boolean = {


This method is used only in one place. Could you inline it there.

OK. I will remove this method.

MaxGekk · 2019-10-18T18:44:03Z

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

+  override def inputTypes: Seq[DataType] = Seq(StringType, StringType)
+
+  override def checkInputDataTypes(): TypeCheckResult = {
+    def checkDecimalPointNum(c: Char): Boolean = {


This one is used only once. I think no need to extract the simple check to a separate function

OK. I will remove this method too.

MaxGekk · 2019-10-18T18:46:31Z

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

 }

+object ToNumber {
+  def transfer(input: UTF8String, pattern: UTF8String): UTF8String = {


Please, rename it to convert()

Good suggestion.

MaxGekk · 2019-10-18T18:51:56Z

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

+      (inputStr, patternStr.replaceAll("FM", ""))
+    }
+    val inputChars = newInputStr.toCharArray()
+    val patternChars = newPatternStr.toIterator


Why do you need to convert it to iterator? String's foreach can iterate over chars as well.

Yes. I should avoid the convert.

MaxGekk · 2019-10-18T18:57:00Z

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

+          case ',' | 'G' if Character.isDigit(currentChar) =>
+          case 'L' =>
+            while (Character.isLetter(inputChars(indexOfString)) ||
+              inputChars(indexOfString) == '$') {


What happens if indexOfString >= inputChars.length?

indexOfString from 0 to inputChars.length - 1. There exists one check.

MaxGekk · 2019-10-18T18:59:42Z

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

+      s"""
+        ${ev.value} = org.apache.spark.sql.catalyst.expressions.ToNumber.transfer(
+          $l, $r);
+       """})


Suggested change

s"""

${ev.value} = org.apache.spark.sql.catalyst.expressions.ToNumber.transfer(

$l, $r);

"""})

s"${ev.value} = org.apache.spark.sql.catalyst.expressions.ToNumber.transfer($l, $r);"

Thanks for your suggestion.

SparkQA · 2019-10-21T06:55:37Z

Test build #112359 has finished for PR 25963 at commit afa19a8.

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

beliefer · 2019-10-23T02:08:46Z

cc @HyukjinKwon @wangyum

beliefer · 2019-10-30T03:29:10Z

cc @gatorsmile @dongjoon-hyun @maropu

maropu · 2019-10-31T23:59:00Z

What's an usecase for this func? Is this useful for users? For example, in the example of this PR description, we can just use an explicit cast like this?

scala> sql("select cast('-12454.8' as double)").show
+------------------------+                                                      
|CAST(-12454.8 AS DOUBLE)|
+------------------------+
|                -12454.8|
+------------------------+

beliefer · 2019-11-01T01:53:06Z

@maropu to_number could support converts format string to number. This is different from cast

beliefer · 2019-11-22T02:08:23Z

@maropu Could you continue to review this PR? Thanks!

maropu · 2020-01-08T04:20:26Z

I'm closing this because of the recent policy for the PostgreSQL dialect. If necessary, please reopen it. Anyway, thanks for the work!

beliefer · 2020-01-09T01:50:01Z

OK. I will wait variety of the policy for the PostgreSQL dialect.

Add postgresql function to_number.

07ad71b

MaxGekk reviewed Sep 29, 2019

View reviewed changes

dongjoon-hyun added the SQL label Sep 29, 2019

Fix scalastyle and remove scala API.

7b4ed77

Fix scalastyle

c4a170c

Fix example issue.

a8286cf

Fix check outputs of expression examples.

2a7202a

convert -> converts

0f51086

MaxGekk reviewed Oct 9, 2019

View reviewed changes

Optimize code.

8deaabf

MaxGekk reviewed Oct 18, 2019

View reviewed changes

Optimize code.

afa19a8

maropu closed this Jan 8, 2020

	-- SELECT '' AS to_number_1, to_number('-34,338,492', '99G999G999');
	-- SELECT '' AS to_number_2, to_number('-34,338,492.654,878', '99G999G999D999G999');
	-- SELECT '' AS to_number_3, to_number('<564646.654564>', '999999.999999PR');
	-- SELECT '' AS to_number_4, to_number('0.00001-', '9.999999S');
	-- SELECT '' AS to_number_5, to_number('5.01-', 'FM9.999999S');
	-- SELECT '' AS to_number_5, to_number('5.01-', 'FM9.999999MI');
	-- SELECT '' AS to_number_7, to_number('5 4 4 4 4 8 . 7 8', '9 9 9 9 9 9 . 9 9');
	-- SELECT '' AS to_number_8, to_number('.01', 'FM9.99');
	-- SELECT '' AS to_number_9, to_number('.0', '99999999.99999999');
	-- SELECT '' AS to_number_10, to_number('0', '99.99');
	-- SELECT '' AS to_number_11, to_number('.-01', 'S99.99');
	-- SELECT '' AS to_number_12, to_number('.01-', '99.99S');
	-- SELECT '' AS to_number_13, to_number(' . 0 1-', ' 9 9 . 9 9 S');
	-- SELECT '' AS to_number_14, to_number('34,50','999,99');
	-- SELECT '' AS to_number_15, to_number('123,000','999G');
	-- SELECT '' AS to_number_16, to_number('123456','999G999');
	-- SELECT '' AS to_number_17, to_number('$1234.56','L9,999.99');
	-- SELECT '' AS to_number_18, to_number('$1234.56','L99,999.99');
	-- SELECT '' AS to_number_19, to_number('$1,234.56','L99,999.99');
	-- SELECT '' AS to_number_20, to_number('1234.56','L99,999.99');
	-- SELECT '' AS to_number_21, to_number('1,234.56','L99,999.99');
	-- SELECT '' AS to_number_22, to_number('42nd', '99th');

	@transient private lazy val formatter: Option[TimestampFormatter] = {
	if (right.foldable) {
	Option(right.eval()).map(format => TimestampFormatter(format.toString, zoneId))
	} else None
	}

	if (patternStr.count(checkDecimalPointNum(_)) > 1) {
	if (patternStr.count(checkDecimalPointNum) > 1) {

	var result = if (integerLen == -1 && wholeLen == -1) {
	val result = if (integerLen == -1 && wholeLen == -1) {

[SPARK-28137][SQL] Add Postgresql function to_number. #25963

[SPARK-28137][SQL] Add Postgresql function to_number. #25963

Uh oh!

Conversation

beliefer commented Sep 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Sep 29, 2019

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Sep 29, 2019

Uh oh!

SparkQA commented Sep 30, 2019

Uh oh!

SparkQA commented Sep 30, 2019

Uh oh!

SparkQA commented Sep 30, 2019

Uh oh!

SparkQA commented Oct 8, 2019

Uh oh!

beliefer commented Oct 8, 2019

Uh oh!

SparkQA commented Oct 8, 2019

Uh oh!

beliefer commented Oct 8, 2019

Uh oh!

SparkQA commented Oct 8, 2019

Uh oh!

beliefer commented Oct 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer commented Sep 29, 2019 •

edited

Loading

beliefer Oct 11, 2019 •

edited

Loading

beliefer Oct 11, 2019 •

edited

Loading

beliefer commented Oct 18, 2019 •

edited

Loading

beliefer commented Oct 30, 2019 •

edited

Loading