[SPARK-32207][SQL] Support 'F'-suffixed Float Literals #29022

yaooqinn · 2020-07-07T08:40:57Z

What changes were proposed in this pull request?

In this PR, I suppose we support 'f'-suffixed float literal, e.g. select 1.1f

Why are the changes needed?

a very common feature across platforms, checked with pg, presto, hive, MySQL...

Does this PR introduce any user-facing change?

yes,

select 1.1f results float value 1.1 instead of throwing AnlaysisExceptiionCan't extract value from 1: need struct type but got int;

How was this patch tested?

add unit tests

yaooqinn · 2020-07-07T08:49:38Z

cc @cloud-fan @maropu @HyukjinKwon thanks for reviewing this in advance.

cloud-fan · 2020-07-07T08:51:00Z

I don't know why it's not supported before, and looks fine to me. cc @maropu @viirya @bart-samwel

bart-samwel · 2020-07-07T08:56:36Z

SGTM!

HyukjinKwon · 2020-07-07T09:36:29Z

+1

maropu · 2020-07-07T10:12:05Z

Oh, I see. Looks okay to add this.

wangyum

Hive 2.3 and PostgreSQL seem to be aliases:

hive> CREATE TABLE test_float1 as SELECT 1.0F, 2.0D;
hive> DESC test_float1;
OK
f                   	decimal(1,0)
_c1                 	double
Time taken: 0.763 seconds, Fetched: 2 row(s)

postgres=# CREATE TABLE test_float1 as SELECT 1.0F, 2.0D;
postgres=# \d test_float1;
            Table "public.test_float1"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 f      | numeric |           |          |
 d      | numeric |           |          |

bart-samwel · 2020-07-07T10:53:21Z

Hive 2.3 and PostgreSQL seem to be aliases:

hive> CREATE TABLE test_float1 as SELECT 1.0F, 2.0D;
hive> DESC test_float1;
OK
f                   	decimal(1,0)
_c1                 	double
Time taken: 0.763 seconds, Fetched: 2 row(s)

postgres=# CREATE TABLE test_float1 as SELECT 1.0F, 2.0D;
postgres=# \d test_float1;
            Table "public.test_float1"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 f      | numeric |           |          |
 d      | numeric |           |          |

I wonder what postgresql does with 1.0e2 -- does it treat it like 1.0 with alias e2 or like 1.0 * 10^2? And 1.0e probably is an alias then. These kinds of discontinuities are terrible and we should stay far away from them...

SparkQA · 2020-07-07T11:12:18Z

Test build #125202 has finished for PR 29022 at commit f260e41.

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

wangyum · 2020-07-07T13:48:02Z

postgres=# CREATE TABLE test_float2 as SELECT 1.0F, 2.0D, 1.0e2, 1.0e;
SELECT 1
postgres=# \d test_float2;
             Table "public.test_float2"
  Column  |  Type   | Collation | Nullable | Default
----------+---------+-----------+----------+---------
 f        | numeric |           |          |
 d        | numeric |           |          |
 ?column? | numeric |           |          |
 e        | numeric |           |          |

postgres=# select * from test_float2;
  f  |  d  | ?column? |  e
-----+-----+----------+-----
 1.0 | 2.0 |      100 | 1.0
(1 row)

SparkQA · 2020-07-07T14:32:47Z

Test build #125206 has finished for PR 29022 at commit 90bb546.

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

viirya · 2020-07-07T16:35:02Z

I noticed this too when I played with other PR. I thought it was by purpose. If it is not, looks okay to add it.

cloud-fan · 2020-07-07T16:38:54Z

retest this please

SparkQA · 2020-07-08T00:13:44Z

Test build #125236 has finished for PR 29022 at commit 90bb546.

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

maropu · 2020-07-08T02:42:47Z

Could you update the doc, too? https://github.com/apache/spark/blob/master/docs/sql-ref-literals.md#numeric-literal
cc: @huaxingao

yaooqinn · 2020-07-08T03:08:08Z

documentation updated, thanks for reminding me @maropu

cloud-fan · 2020-07-08T03:26:40Z

docs/sql-ref-literals.md


    Case insensitive, indicates `DOUBLE`, which is an 8-byte double-precision floating point number.

+* **D**


maropu

LGTM except for the @cloud-fan comment.

dongjoon-hyun · 2020-07-08T04:12:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

      Long.MinValue, Long.MaxValue, LongType.simpleString)(_.toLong)
  }

+  override def visitFloatLiteral(ctx: FloatLiteralContext): Literal = {


Could you add a function description like the other functions around here, @yaooqinn ?

done, thanks!

dongjoon-hyun

+1, LGTM (except @cloud-fan 's comment and mine)

SparkQA · 2020-07-08T04:47:22Z

Test build #125287 has finished for PR 29022 at commit 4439bb6.

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

HyukjinKwon · 2020-07-08T04:52:40Z

retest this please

wangyum · 2020-07-08T06:19:07Z

Maybe we should convert suffix to alias. Please see these examples for more details:

Presto:

MySQL 5.7.26:

MySQL 8.0.16:

MariaDB 10.4.6:

PostgreSQL 11.3:

Hive 3.1.2:

yaooqinn · 2020-07-08T06:34:13Z

Maybe we should convert suffix to alias. Please see these examples for more details:

If the digits part is integral, e.g. 2f, 2a, these systems vary from each other..., not always alias. on the other hand, inside spark, we have different error msg for 2.1f(Can't extract value from 1: need struct type but got int;) and 2f(cannot resolve '2f' given input columns: []), but 2d and 2.1d are valid inputs. IMHO, it's better to keep the consistency of our
own first.

wangyum · 2020-07-08T06:43:14Z

Is there a system support F-suffixed as float Literal?

SparkQA · 2020-07-08T07:05:02Z

Test build #125298 has finished for PR 29022 at commit 4439bb6.

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

SparkQA · 2020-07-08T07:05:05Z

Test build #125307 has finished for PR 29022 at commit 6e422ff.

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

cloud-fan · 2020-07-08T07:22:21Z

retest this please

cloud-fan · 2020-07-08T07:23:17Z

Seems like most databases don't support the D suffix. Spark can't drop the D support now, I think it makes more sense to be consistent and support F as well.

SparkQA · 2020-07-08T07:27:59Z

Test build #125320 has started for PR 29022 at commit 6e422ff.

maropu · 2020-07-08T07:59:21Z

Yea, the @cloud-fan comment above sounds fine to me.

dongjoon-hyun · 2020-07-08T15:55:53Z

Retest this please.

SparkQA · 2020-07-08T17:25:27Z

Test build #125384 has finished for PR 29022 at commit 6e422ff.

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

cloud-fan · 2020-07-08T17:44:30Z

retest this please

yaooqinn · 2020-07-08T17:46:15Z

retest this please

SparkQA · 2020-07-08T23:33:38Z

Test build #125388 has finished for PR 29022 at commit 6e422ff.

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

yaooqinn · 2020-07-09T02:08:26Z

retest this please

SparkQA · 2020-07-09T04:36:29Z

Test build #125425 has finished for PR 29022 at commit 6e422ff.

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

yaooqinn · 2020-07-09T05:25:01Z

retest this please

SparkQA · 2020-07-09T07:05:03Z

Test build #125435 has finished for PR 29022 at commit 6e422ff.

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

HyukjinKwon · 2020-07-09T07:14:20Z

retest this please

SparkQA · 2020-07-09T15:03:00Z

Test build #125443 has finished for PR 29022 at commit 6e422ff.

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

cloud-fan · 2020-07-09T15:41:58Z

retest this please

SparkQA · 2020-07-09T15:46:53Z

Test build #125491 has started for PR 29022 at commit 6e422ff.

yaooqinn · 2020-07-09T16:41:25Z

retest this please

dongjoon-hyun · 2020-07-09T17:59:05Z

Retest this please

SparkQA · 2020-07-10T01:51:29Z

Test build #125505 has finished for PR 29022 at commit 6e422ff.

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

dongjoon-hyun · 2020-07-10T02:44:58Z

Thank you, @yaooqinn and all. Merged to master.

yaooqinn · 2020-07-10T02:51:01Z

Thanks for merging @dongjoon-hyun and all for the help

[SPARK-32207][SQL] Support 'F'-suffixed Float Literals

f260e41

probot-autolabeler bot added the SQL label Jul 7, 2020

wangyum reviewed Jul 7, 2020

View reviewed changes

nit

90bb546

doc

0f9feb6

probot-autolabeler bot added the DOCS label Jul 8, 2020

nit

4439bb6

cloud-fan reviewed Jul 8, 2020

View reviewed changes

maropu approved these changes Jul 8, 2020

View reviewed changes

dongjoon-hyun reviewed Jul 8, 2020

View reviewed changes

dongjoon-hyun approved these changes Jul 8, 2020

View reviewed changes

addr comments

6e422ff

dongjoon-hyun closed this in 4609f1f Jul 10, 2020


		Case insensitive, indicates `DOUBLE`, which is an 8-byte double-precision floating point number.

		* D

[SPARK-32207][SQL] Support 'F'-suffixed Float Literals #29022

[SPARK-32207][SQL] Support 'F'-suffixed Float Literals #29022

Uh oh!

Conversation

yaooqinn commented Jul 7, 2020 • 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!

yaooqinn commented Jul 7, 2020

Uh oh!

cloud-fan commented Jul 7, 2020

Uh oh!

bart-samwel commented Jul 7, 2020

Uh oh!

HyukjinKwon commented Jul 7, 2020

Uh oh!

maropu commented Jul 7, 2020

Uh oh!

wangyum left a comment

Choose a reason for hiding this comment

Uh oh!

bart-samwel commented Jul 7, 2020

Uh oh!

SparkQA commented Jul 7, 2020

Uh oh!

wangyum commented Jul 7, 2020

Uh oh!

SparkQA commented Jul 7, 2020

Uh oh!

viirya commented Jul 7, 2020

Uh oh!

cloud-fan commented Jul 7, 2020

Uh oh!

SparkQA commented Jul 8, 2020

Uh oh!

maropu commented Jul 8, 2020

Uh oh!

yaooqinn commented Jul 8, 2020

Uh oh!

cloud-fan Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

yaooqinn Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 8, 2020

Uh oh!

HyukjinKwon commented Jul 8, 2020

Uh oh!

wangyum commented Jul 8, 2020

Uh oh!

yaooqinn commented Jul 8, 2020

Uh oh!

wangyum commented Jul 8, 2020

Uh oh!

SparkQA commented Jul 8, 2020

Uh oh!

SparkQA commented Jul 8, 2020

Uh oh!

cloud-fan commented Jul 8, 2020

Uh oh!

cloud-fan commented Jul 8, 2020

Uh oh!

SparkQA commented Jul 8, 2020

Uh oh!

maropu commented Jul 8, 2020

Uh oh!

dongjoon-hyun commented Jul 8, 2020

yaooqinn commented Jul 7, 2020 •

edited

Loading