-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CALCITE-6560] Add supportsNegativeScale in RelDataTypeSystem #3945
Conversation
core/src/main/java/org/apache/calcite/sql/type/SqlTypeFactoryImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/main/java/org/apache/calcite/rel/type/DelegatingTypeSystem.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
Outdated
Show resolved
Hide resolved
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
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
Outdated
Show resolved
Hide resolved
@mihaibudiu If we have a minimum scale value, do we still need supportsNegativeScale? If the minimum scale value <0 means support the negative scale. |
I think it's still useful as a method, it's nicer than comparing the minimum scale with 0. |
Maybe one method can delegate to the other. To define a type system that allows negative scale, people should only override the
|
core/src/main/java/org/apache/calcite/sql/type/SqlTypeFactoryImpl.java
Outdated
Show resolved
Hide resolved
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))", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is 0 the right result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I want to cover the INTERVAL type. But I can't find a database to reference.
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should say that the default type system returns 0,
and that there are no 'special values' (like SCALE_NOT_SPECIFIED).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
.
@julianhyde I have made some improvements based on your suggestions.
|
c4eff23
to
fa0dd90
Compare
Quality Gate passedIssues Measures |
No description provided.