[CALCITE-4240] SqlTypeUtil#getMaxPrecisionScaleDecimal returns a decimal that with same precision and scale (Jiatao Tao) #2161

Aaaaaaron · 2020-09-21T03:10:18Z

Hi @danny0405 , can you take a look at this PR~

"maxScale" should not greater than "maxPrecision". If they are equal, e.g. Decimal(19,19) means we can only have decimal places.

Aaaaaaron · 2020-09-21T03:20:38Z

Still have some ut failed, will fix than, with review comments.

julianhyde · 2020-09-21T21:41:02Z

Remove the word 'fix' from the commit message. Change the message so that it is clear what the bug is.

Add space before '(' in commit message.

Why would DECIMAL(10, 15) not be valid? Say if you are measuring a small quantify such as the Gravitational constant, 6.674 * 10^-11.

Aaaaaaron · 2020-09-22T06:32:44Z

Remove the word 'fix' from the commit message. Change the message so that it is clear what the bug is.

Add space before '(' in commit message.

Why would DECIMAL(10, 15) not be valid? Say if you are measuring a small quantify such as the Gravitational constant, 6.674 * 10-11.

Hi @julianhyde , Roger the message irregular, will refine latter.

From my understanding, precision represents the total number of digits that can be represented by the column, so it must >= than scale.
DECIMAL(15, 10) means that the total number of digits is 15, and the decimal place is 10(in other words, the number of integer place is 5).

If you execute create table test (a decimal(1,3)); in MySQL, will occur error: Schema Error: Error: ER_M_BIGGER_THAN_D: For float(M,D), double(M,D) or decimal(M,D), M must be >= D (column 'a').

Also in Spark/hive:

scala> spark.sql("CREATE TABLE foo (a DECIMAL(5, 7))").collect
org.apache.spark.sql.catalyst.parser.ParseException:
Decimal scale (7) cannot be greater than precision (5).

hive> CREATE TABLE foo (a DECIMAL(5, 7));
2020-09-22 14:38:55,062	FAILED: IllegalArgumentException Decimal scale must be less than or equal to precision

danny0405 · 2020-09-23T02:04:42Z

core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java

+    // means we can only have decimal places.
+    while (maxScale >= maxPrecision) {
+      maxScale = maxScale / 2;
+    }


I'm afraid not all the sql engines have the rule that maxScale = maxScale / 2, we should add a check for the validity instead of modifying the value silently. And i'm inclined to fix the default max precision/scale for Calcite type system.

I'm afraid not all the sql engines have the rule that maxScale = maxScale / 2, we should add a check for the validity instead of modifying the value silently. And i'm inclined to fix the default max precision/scale for Calcite type system.

This is also a proposal, I'll take a look at Hive/Spark decimal's max scale/repcision, but I'm afraid they may be the same, or we don't get max but get default?

Spark

Default: (10, 0)
Max: The precision can be up to 38, scale can also be up to 38 (less or equal to precision).

https://spark.apache.org/docs/latest/api/java/org/apache/spark/sql/types/DecimalType.html

Hive

Default: (10, 0)
Max: Decimal precision out of allowed range [1,38], Decimal scale out of allowed range [0,38]

https://cwiki.apache.org/confluence/display/Hive/LanguageManual+Types

The "getMaxPrecisionScaleDecimal" means suppose to get a max Decimal, but the Decimal(max, max) is not this meaning.

I got the impression that many systems support max precision scale as decimal(38, 18).

I got the impression that many systems support max precision scale as decimal(38, 18).

Hi @danny0405, IMO the specific number here is not so important (like18 or 19), we just need return a big decimal here, so can we finalize a number just like Decimal(38, 18)?

cc @julianhyde

Let's just make the valid max scale half of the max precision, e.g. the max scale = max precision/2.

Let's just make the valid max scale half of the max precision, e.g. the max scale = max precision/2.

OK

danny0405 · 2020-09-29T05:51:14Z

Thanks, can you fix the conflicts.

danny0405 · 2020-10-10T06:53:21Z

The tests still fails.

Aaaaaaron · 2020-10-10T08:57:18Z

The tests still fails.

I'm working on this 😂

Aaaaaaron · 2020-10-10T09:08:31Z

core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java

    expr("'12.3' * '5'")
-        .columnType("DECIMAL(19, 19) NOT NULL");
+        .columnType("DECIMAL(19, 18) NOT NULL");


here comes the 18

Aaaaaaron · 2020-10-10T09:13:21Z

core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java

    expr("12.3/'5.1'")
-        .columnType("DECIMAL(19, 0) NOT NULL");
+        .columnType("DECIMAL(19, 8) NOT NULL");


here comes the 8, by the way, the origin scale 0 is not reasonable

…mal that with same precision and scale (Jiatao Tao)

Aaaaaaron · 2020-10-11T03:53:53Z

Hi @danny0405 the ut passed, would you take a look?

danny0405

+1

…mal that with same precision and scale (Jiatao Tao) The SqlTypeUtil#getMaxPrecisionScaleDecimal now returns decimal type with max precision and scale half of that. Previously it returns DECIMAL(19, 19) which is invalid. close apache#2161

Aaaaaaron changed the title ~~[CALCITE-4240] Fix SqlTypeUtil#getMaxPrecisionScaleDecimal returns a decimal that with same precision and scale(Jiatao Tao)~~ [CALCITE-4240] SqlTypeUtil#getMaxPrecisionScaleDecimal returns a decimal that with same precision and scale (Jiatao Tao) Sep 22, 2020

Aaaaaaron force-pushed the CALCITE-4240 branch from b589272 to d510178 Compare September 22, 2020 06:49

danny0405 reviewed Sep 23, 2020

View reviewed changes

Aaaaaaron force-pushed the CALCITE-4240 branch 4 times, most recently from 857dcc6 to 0ef386c Compare October 10, 2020 02:55

Aaaaaaron force-pushed the CALCITE-4240 branch from 0ef386c to fe24721 Compare October 10, 2020 09:04

Aaaaaaron commented Oct 10, 2020

View reviewed changes

Aaaaaaron force-pushed the CALCITE-4240 branch from fe24721 to 0c9ec90 Compare October 10, 2020 14:28

[CALCITE-4240] SqlTypeUtil#getMaxPrecisionScaleDecimal returns a deci…

b0fdb37

…mal that with same precision and scale (Jiatao Tao)

Aaaaaaron force-pushed the CALCITE-4240 branch from 0c9ec90 to b0fdb37 Compare October 10, 2020 16:13

vlsi force-pushed the master branch from 5a338ff to 8df4df0 Compare October 10, 2020 16:14

Aaaaaaron requested a review from danny0405 October 11, 2020 03:53

danny0405 added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Oct 13, 2020

danny0405 reviewed Oct 13, 2020

View reviewed changes

danny0405 approved these changes Oct 13, 2020

View reviewed changes

danny0405 added the slow-tests-needed label Oct 14, 2020

danny0405 closed this in 16b22b1 Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-4240] SqlTypeUtil#getMaxPrecisionScaleDecimal returns a decimal that with same precision and scale (Jiatao Tao) #2161

[CALCITE-4240] SqlTypeUtil#getMaxPrecisionScaleDecimal returns a decimal that with same precision and scale (Jiatao Tao) #2161

Aaaaaaron commented Sep 21, 2020 •

edited

Loading

Aaaaaaron commented Sep 21, 2020

julianhyde commented Sep 21, 2020 •

edited

Loading

Aaaaaaron commented Sep 22, 2020 •

edited

Loading

danny0405 Sep 23, 2020

Aaaaaaron Sep 23, 2020

Aaaaaaron Sep 23, 2020 •

edited

Loading

danny0405 Sep 23, 2020

Aaaaaaron Sep 23, 2020

Aaaaaaron Sep 23, 2020

danny0405 Sep 28, 2020

Aaaaaaron Sep 28, 2020

danny0405 commented Sep 29, 2020

danny0405 commented Oct 10, 2020

Aaaaaaron commented Oct 10, 2020

Aaaaaaron Oct 10, 2020

Aaaaaaron Oct 10, 2020

Aaaaaaron commented Oct 11, 2020

danny0405 left a comment

[CALCITE-4240] SqlTypeUtil#getMaxPrecisionScaleDecimal returns a decimal that with same precision and scale (Jiatao Tao) #2161

[CALCITE-4240] SqlTypeUtil#getMaxPrecisionScaleDecimal returns a decimal that with same precision and scale (Jiatao Tao) #2161

Conversation

Aaaaaaron commented Sep 21, 2020 • edited Loading

Aaaaaaron commented Sep 21, 2020

julianhyde commented Sep 21, 2020 • edited Loading

Aaaaaaron commented Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaaaaaron Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Spark

Hive

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danny0405 commented Sep 29, 2020

danny0405 commented Oct 10, 2020

Aaaaaaron commented Oct 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaaaaaron commented Oct 11, 2020

danny0405 left a comment

Choose a reason for hiding this comment

Aaaaaaron commented Sep 21, 2020 •

edited

Loading

julianhyde commented Sep 21, 2020 •

edited

Loading

Aaaaaaron commented Sep 22, 2020 •

edited

Loading

Aaaaaaron Sep 23, 2020 •

edited

Loading