[CALCITE-6560] Add supportsNegativeScale in RelDataTypeSystem #3945

NobiGo · 2024-09-02T23:27:30Z

No description provided.

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

core/src/test/resources/sql/misc.iq

julianhyde

Can you add to SqlParserTest?

Also add to either SqlValidatorTest or SqlOperatorTest. The datatype occurs in a call to CAST. Make sure that a CAST is valid if and only if the type system allows negative types.

Do you expect to be able to evaluate queries with negative scales? Cast to a negative scale will result in a shifted decimal, e.g. 12,300 represented as the int 123 but printed as 12300.

core/src/test/resources/sql/misc.iq

core/src/main/java/org/apache/calcite/rel/type/DelegatingTypeSystem.java

core/src/main/codegen/templates/Parser.jj

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java

NobiGo · 2024-09-04T23:22:36Z

Can you add to SqlParserTest?

Also add to either SqlValidatorTest or SqlOperatorTest. The datatype occurs in a call to CAST. Make sure that a CAST is valid if and only if the type system allows negative types.

Do you expect to be able to evaluate queries with negative scales? Cast to a negative scale will result in a shifted decimal, e.g. 12,300 represented as the int 123 but printed as 12300.

We have created an ISSUE to support this https://issues.apache.org/jira/browse/CALCITE-6406. I plan to do this in CALCITE-6406.

core/src/test/java/org/apache/calcite/sql/type/RelDataTypeSystemTest.java

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java

NobiGo · 2024-09-13T14:25:12Z

@mihaibudiu If we have a minimum scale value, do we still need supportsNegativeScale? If the minimum scale value <0 means support the negative scale.

mihaibudiu · 2024-09-13T20:13:05Z

I think it's still useful as a method, it's nicer than comparing the minimum scale with 0.

julianhyde · 2024-09-13T20:26:57Z

Maybe one method can delegate to the other. To define a type system that allows negative scale, people should only override the minimumScale method. supportsNegativeScale would delegate to it:

default boolean supportsNegativeScale() {
  return minimumScale() < 0;
}

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

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

core/src/test/resources/sql/cast.iq

mihaibudiu · 2024-09-20T16:40:26Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+    negativeScaleFixture.checkScalar("cast('12.3' as decimal(3, -1))",
+        "10", "DECIMAL(3, -1) NOT NULL");
+    // cast interval to decimal
+    negativeScaleFixture.checkScalar("cast(INTERVAL '5' hour as decimal(3, -1))",


is 0 the right result?

@mihaibudiu According to test in testCastIntervalToNumeric

f.checkScalarExact("cast(INTERVAL '5' minute as decimal(2,1))", "DECIMAL(2, 1) NOT NULL", isDecimal("5.0")); f.checkScalarExact("cast(INTERVAL '5' hour as decimal(2,1))", "DECIMAL(2, 1) NOT NULL", isDecimal("5.0"));

So I think it is right. WDYT?

frankly, I don't think that this particular test is that useful.
it depends on the semantics of converting intervals to decimals, which I am not sure is defined in the same way in various SQL dialects.
so you are essentially testing casting 5.0 to DECIMAL(3,-1), right?

Yes. I want to cover the INTERVAL type. But I can't find a database to reference.

julianhyde · 2024-09-20T16:51:29Z

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java

@@ -62,6 +64,14 @@ public interface RelDataTypeSystem {
  /** Returns the maximum precision of a NUMERIC or DECIMAL type. */
  int getMaxNumericPrecision();

+  /** Returns the minimum scale of a NUMERIC or DECIMAL type. */
+  int getMinNumericScale();


You should say that the default type system returns 0,
and that there are no 'special values' (like SCALE_NOT_SPECIFIED).

RelDataTypeSystem is an interface It's strange to add a Java document here. So I added it in RelDataTypeSystemImpl. We have a SCALE_NOT_SPECIFIED in RelDataType for the types where the scale is not allowed.

julianhyde · 2024-09-20T16:57:28Z

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java

@@ -62,6 +64,14 @@ public interface RelDataTypeSystem {
  /** Returns the maximum precision of a NUMERIC or DECIMAL type. */
  int getMaxNumericPrecision();

+  /** Returns the minimum scale of a NUMERIC or DECIMAL type. */
+  int getMinNumericScale();


We currently have both getMaxScale(SqlTypeName) and getMaxNumericScale(). (The latter delegates to the former.)

So should we instead be creating getMinScale(SqlTypeName)? It would allow a type system to have different min scales for (say) a decimal and a binary floating point.

Yes. I have also noticed this, and we already have a method that returns a fixed value in SqlTypeName#getMinScale. I think we can do it.

I checked the SQL TypeName#getMinScale and it processed the Interval type, which returns -1 for all other types. Do we need to add this method?

@julianhyde

We should be explicit in the javadoc.

getMaxScale(SqlTypeName) seems to return RelDataType.SCALE_NOT_SPECIFIED (Integer.MIN_VALUE) for any data type that does not support scale. Can you amend its javadoc to state this?

The javadoc for getMinScale(SqlTypeName) should state its behavior for types that do not support scale. I propose Integer.MAX_VALUE.

The javadoc for getMinScale(SqlTypeName) should also say that the default type system returns 0 for DECIMAL and interval types. I know it's an interface, but interfaces can and should say what the default or typical behavior is.

I guess you should change supportsNegativeScale() to supportsNegativeScale(SqlTypeName), and remove getMinNumericScale().

NobiGo · 2024-09-21T09:02:57Z

@julianhyde I have made some improvements based on your suggestions.

Refactor SqlTypeName#getDefaultScale to RelDataTypeSystem#getDefaultScale(SqlTypeName) and mark SqlTypeName#getDefaultScale as deprecated.
Refactor SqlTypeName#getMinPrecision to RelDataTypeSystem#getMinPrecision(SqlTypeName) and mark SqlTypeName#getMinPrecision as deprecated.
Refactor SqlTypeName#getMinScale to RelDataTypeSystem#getMinScale(SqlTypeName) and mark SqlTypeName#getMinScale as deprecated.
Add Javadoc in RelDataTypeSystem#getMaxScale, RelDataTypeSystem#getMinScale, RelDataTypeSystem#getDefaultScale, and RelDataTypeSystem#getMinPrecision.
Add getMinNumericScale in RelDataTypeSystem. getMinScale will use it when extracting decimal min scale（Same as getMaxPrecision(SqlTypeName) and getMaxNumericPrecision).
I use RelDataType.SCALE_NOT_SPECIFIED as the default minimum scale for the type that doesn’t support scale. Not the Integer.MAX_VALUE. Because I think
RelDataType.SCALE_NOT_SPECIFIED is enough and makes the code logically simple.

sonarcloud · 2024-09-22T01:34:07Z

Quality Gate passed

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mihaibudiu reviewed Sep 2, 2024

View reviewed changes

core/src/main/java/org/apache/calcite/sql/type/SqlTypeFactoryImpl.java Outdated Show resolved Hide resolved

core/src/test/resources/sql/misc.iq Outdated Show resolved Hide resolved

NobiGo requested a review from mihaibudiu September 4, 2024 03:41

mihaibudiu approved these changes Sep 4, 2024

View reviewed changes

mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 4, 2024

NobiGo force-pushed the CALCITE-6560 branch from 4f84ad4 to fb40c86 Compare September 4, 2024 13:55

julianhyde reviewed Sep 4, 2024

View reviewed changes

NobiGo force-pushed the CALCITE-6560 branch from 912c77a to f21da46 Compare September 5, 2024 14:29

NobiGo removed the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 6, 2024

NobiGo marked this pull request as draft September 6, 2024 12:55

NobiGo force-pushed the CALCITE-6560 branch from f21da46 to 43a433b Compare September 8, 2024 01:55

NobiGo marked this pull request as ready for review September 8, 2024 02:37

NobiGo requested review from julianhyde and mihaibudiu September 8, 2024 02:37

mihaibudiu reviewed Sep 12, 2024

View reviewed changes

core/src/test/java/org/apache/calcite/sql/type/RelDataTypeSystemTest.java Outdated Show resolved Hide resolved

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java Outdated Show resolved Hide resolved

mihaibudiu reviewed Sep 20, 2024

View reviewed changes

julianhyde reviewed Sep 20, 2024

View reviewed changes

NobiGo added 6 commits September 22, 2024 06:55

[CALCITE-6560] Add supportsNegativeScale in RelDataTypeSystem

89a8122

Supports negative scale

e4d6675

[CALCITE-6560] Support the minimum scale

2231e75

[CALCITE-6560] Code review about SqlTypeFactoryImpl

a323f87

[CALCITE-6560] Code review about RelDataTypeSystemImpl

0646f2d

[CALCITE-6560] Code Review about the javadoc

fa0dd90

NobiGo force-pushed the CALCITE-6560 branch from c4eff23 to fa0dd90 Compare September 21, 2024 23:51

[CALCITE-6560] Make Test Pass

6de2916

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-6560] Add supportsNegativeScale in RelDataTypeSystem #3945

[CALCITE-6560] Add supportsNegativeScale in RelDataTypeSystem #3945

NobiGo commented Sep 2, 2024

julianhyde left a comment

NobiGo commented Sep 4, 2024

NobiGo commented Sep 13, 2024

mihaibudiu commented Sep 13, 2024

julianhyde commented Sep 13, 2024

mihaibudiu Sep 20, 2024

NobiGo Sep 20, 2024

mihaibudiu Sep 20, 2024

NobiGo Sep 20, 2024

julianhyde Sep 20, 2024

NobiGo Sep 20, 2024

julianhyde Sep 20, 2024

NobiGo Sep 20, 2024

NobiGo Sep 20, 2024

NobiGo Sep 20, 2024

julianhyde Sep 20, 2024

NobiGo commented Sep 21, 2024

sonarcloud bot commented Sep 22, 2024

[CALCITE-6560] Add supportsNegativeScale in RelDataTypeSystem #3945

Are you sure you want to change the base?

[CALCITE-6560] Add supportsNegativeScale in RelDataTypeSystem #3945

Conversation

NobiGo commented Sep 2, 2024

julianhyde left a comment

Choose a reason for hiding this comment

NobiGo commented Sep 4, 2024

NobiGo commented Sep 13, 2024

mihaibudiu commented Sep 13, 2024

julianhyde commented Sep 13, 2024

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

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

Choose a reason for hiding this comment

NobiGo commented Sep 21, 2024

sonarcloud bot commented Sep 22, 2024

Quality Gate passed