Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[CALCITE-6560] Add supportsNegativeScale in RelDataTypeSystem #3945

Closed
wants to merge 8 commits into from
+814 −43
Closed

[CALCITE-6560] Add supportsNegativeScale in RelDataTypeSystem #3945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
89a8122
[CALCITE-6560] Add supportsNegativeScale in RelDataTypeSystem
NobiGo Sep 21, 2024
e4d6675
Supports negative scale
NobiGo Sep 6, 2024
2231e75
[CALCITE-6560] Support the minimum scale
NobiGo Sep 21, 2024
a323f87
[CALCITE-6560] Code review about SqlTypeFactoryImpl
NobiGo Sep 20, 2024
0646f2d
[CALCITE-6560] Code review about RelDataTypeSystemImpl
NobiGo Sep 20, 2024
fa0dd90
[CALCITE-6560] Code Review about the javadoc
NobiGo Sep 21, 2024
6de2916
[CALCITE-6560] Make Test Pass
NobiGo Sep 22, 2024
3178e64
[CALCITE-6560] Rerun CI
NobiGo Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions core/src/main/codegen/templates/Parser.jj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5930,8 +5930,8 @@ SqlTypeNameSpec SqlTypeName2(Span s) :
SqlTypeNameSpec SqlTypeName3(Span s) :
{
final SqlTypeName sqlTypeName;
int precision = -1;
int scale = -1;
int precision = RelDataType.PRECISION_NOT_SPECIFIED;
int scale = RelDataType.SCALE_NOT_SPECIFIED;
}
{
(
Expand All @@ -5944,7 +5944,7 @@ SqlTypeNameSpec SqlTypeName3(Span s) :
precision = UnsignedIntLiteral()
[
<COMMA>
scale = UnsignedIntLiteral()
scale = IntLiteral()
NobiGo marked this conversation as resolved.
Show resolved Hide resolved
]
<RPAREN>
]
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/calcite/jdbc/CalciteMetaImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ private ImmutableList<MetaTypeInfo> getAllDefaultType() {
false,
false,
typeSystem.isAutoincrement(sqlTypeName),
(short) sqlTypeName.getMinScale(),
(short) typeSystem.getMinScale(sqlTypeName),
(short) typeSystem.getMaxScale(sqlTypeName),
typeSystem.getNumTypeRadix(sqlTypeName)));
}
Expand Down
16 changes: 16 additions & 0 deletions core/src/main/java/org/apache/calcite/rel/type/DelegatingTypeSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,26 @@ protected DelegatingTypeSystem(RelDataTypeSystem typeSystem) {
return typeSystem.getMaxScale(typeName);
}

@Override public int getMinScale(SqlTypeName typeName) {
return typeSystem.getMinScale(typeName);
}

@Override public int getDefaultPrecision(SqlTypeName typeName) {
return typeSystem.getDefaultPrecision(typeName);
}

@Override public int getDefaultScale(SqlTypeName typeName) {
return typeSystem.getDefaultScale(typeName);
}

@Override public int getMaxPrecision(SqlTypeName typeName) {
return typeSystem.getMaxPrecision(typeName);
}

@Override public int getMinPrecision(SqlTypeName typeName) {
return typeSystem.getMinPrecision(typeName);
}

@Override public int getMaxNumericScale() {
return typeSystem.getMaxNumericScale();
}
Expand All @@ -52,6 +64,10 @@ protected DelegatingTypeSystem(RelDataTypeSystem typeSystem) {
return typeSystem.getMaxNumericPrecision();
}

@Override public int getMinNumericScale() {
return typeSystem.getMinNumericScale();
}

@Override public RoundingMode roundingMode() {
return typeSystem.roundingMode();
}
Expand Down
52 changes: 45 additions & 7 deletions core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,31 +37,69 @@ public interface RelDataTypeSystem {
/** Default type system. */
RelDataTypeSystem DEFAULT = new RelDataTypeSystemImpl() { };

/** Returns the maximum scale of a given type. */
/**
* Returns the maximum scale allowed for this type, or
* {@link RelDataType#SCALE_NOT_SPECIFIED}
* if scale is not applicable for this type.
*
* @return Maximum allowed scale
*/
int getMaxScale(SqlTypeName typeName);

/**
* Returns default precision for this type if supported, otherwise -1 if
* precision is either unsupported or must be specified explicitly.
* Returns the minimum scale allowed for this type, or
* {@link RelDataType#SCALE_NOT_SPECIFIED}
* if scale are not applicable for this type.
*
* @return Minimum allowed scale
*/
int getMinScale(SqlTypeName typeName);

/**
* Returns default precision for this type if supported, otherwise
* {@link RelDataType#PRECISION_NOT_SPECIFIED}
* if precision is either unsupported or must be specified explicitly.
*
* @return Default precision
*/
int getDefaultPrecision(SqlTypeName typeName);

/**
* Returns the maximum precision (or length) allowed for this type, or -1 if
* precision/length are not applicable for this type.
* Returns default scale for this type if supported, otherwise
* {@link RelDataType#SCALE_NOT_SPECIFIED}
* if scale is either unsupported or must be specified explicitly.
*
* @return Default scale
*/
int getDefaultScale(SqlTypeName typeName);

/**
* Returns the maximum precision (or length) allowed for this type, or
* {@link RelDataType#PRECISION_NOT_SPECIFIED}
* if precision/length are not applicable for this type.
*
* @return Maximum allowed precision
*/
int getMaxPrecision(SqlTypeName typeName);

/** Returns the maximum scale of a NUMERIC or DECIMAL type. */
/**
* Returns the minimum precision (or length) allowed for this type, or
* {@link RelDataType#PRECISION_NOT_SPECIFIED}
* if precision/length are not applicable for this type.
*
* @return Minimum allowed precision
*/
int getMinPrecision(SqlTypeName typeName);

/** Returns the maximum scale of a NUMERIC or DECIMAL type. And the default value is 19. */
int getMaxNumericScale();

/** Returns the maximum precision of a NUMERIC or DECIMAL type. */
/** Returns the maximum precision of a NUMERIC or DECIMAL type. And the default value is 19. */
int getMaxNumericPrecision();

/** Returns the minimum scale of a NUMERIC or DECIMAL type. And the default value is 0. */
int getMinNumericScale();
Copy link
Contributor

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).

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julianhyde

Copy link
Contributor

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().


/** Returns the rounding behavior for numerical operations capable of discarding precision. */
RoundingMode roundingMode();

Expand Down
101 changes: 101 additions & 0 deletions core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystemImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
* <caption>Parameter values</caption>
* <tr><th>Parameter</th> <th>Value</th></tr>
* <tr><td>MAX_NUMERIC_SCALE</td> <td>19</td></tr>
* <tr><td>MIN_NUMERIC_SCALE</td> <td>0</td></tr>
* <tr><td>MAX_NUMERIC_PRECISION</td> <td>19</td></tr>
* </table>
*/
public abstract class RelDataTypeSystemImpl implements RelDataTypeSystem {
Expand All @@ -61,6 +63,36 @@ public abstract class RelDataTypeSystemImpl implements RelDataTypeSystem {
}
}

/**
* Returns the minimum scale (or fractional second precision in the case of
* intervals) allowed for this type, or {@link RelDataType#SCALE_NOT_SPECIFIED}
* if precision/length are not applicable for this type.
*
* @return Minimum allowed scale
*/
@Override public int getMinScale(SqlTypeName typeName) {
switch (typeName) {
case DECIMAL:
return getMinNumericScale();
case INTERVAL_YEAR:
case INTERVAL_YEAR_MONTH:
case INTERVAL_MONTH:
case INTERVAL_DAY:
case INTERVAL_DAY_HOUR:
case INTERVAL_DAY_MINUTE:
case INTERVAL_DAY_SECOND:
case INTERVAL_HOUR:
case INTERVAL_HOUR_MINUTE:
case INTERVAL_HOUR_SECOND:
case INTERVAL_MINUTE:
case INTERVAL_MINUTE_SECOND:
case INTERVAL_SECOND:
return SqlTypeName.MIN_INTERVAL_FRACTIONAL_SECOND_PRECISION;
default:
return RelDataType.SCALE_NOT_SPECIFIED;
}
}

@Override public int getDefaultPrecision(SqlTypeName typeName) {
// Following BasicSqlType precision as the default
switch (typeName) {
Expand Down Expand Up @@ -118,6 +150,31 @@ public abstract class RelDataTypeSystemImpl implements RelDataTypeSystem {
}
}

/** Returns the default scale for this type if supported, otherwise {@link RelDataType#SCALE_NOT_SPECIFIED}
* if scale is either unsupported or must be specified explicitly. */
@Override public int getDefaultScale(SqlTypeName typeName) {
switch (typeName) {
case DECIMAL:
return 0;
case INTERVAL_YEAR:
case INTERVAL_YEAR_MONTH:
case INTERVAL_MONTH:
case INTERVAL_DAY:
case INTERVAL_DAY_HOUR:
case INTERVAL_DAY_MINUTE:
case INTERVAL_DAY_SECOND:
case INTERVAL_HOUR:
case INTERVAL_HOUR_MINUTE:
case INTERVAL_HOUR_SECOND:
case INTERVAL_MINUTE:
case INTERVAL_MINUTE_SECOND:
case INTERVAL_SECOND:
return SqlTypeName.DEFAULT_INTERVAL_FRACTIONAL_SECOND_PRECISION;
default:
return RelDataType.SCALE_NOT_SPECIFIED;
}
}

@Override public int getMaxPrecision(SqlTypeName typeName) {
switch (typeName) {
case DECIMAL:
Expand Down Expand Up @@ -154,6 +211,46 @@ public abstract class RelDataTypeSystemImpl implements RelDataTypeSystem {
}
}

/**
* Returns the minimum precision (or length) allowed for this type,
* or {@link RelDataType#PRECISION_NOT_SPECIFIED}
* if precision/length are not applicable for this type.
*
* @return Minimum allowed precision
*/
@Override public int getMinPrecision(SqlTypeName typeName) {
switch (typeName) {
case DECIMAL:
case VARCHAR:
case CHAR:
case VARBINARY:
case BINARY:
case TIME:
case TIME_WITH_LOCAL_TIME_ZONE:
case TIME_TZ:
case TIMESTAMP:
case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
case TIMESTAMP_TZ:
return 1;
case INTERVAL_YEAR:
case INTERVAL_YEAR_MONTH:
case INTERVAL_MONTH:
case INTERVAL_DAY:
case INTERVAL_DAY_HOUR:
case INTERVAL_DAY_MINUTE:
case INTERVAL_DAY_SECOND:
case INTERVAL_HOUR:
case INTERVAL_HOUR_MINUTE:
case INTERVAL_HOUR_SECOND:
case INTERVAL_MINUTE:
case INTERVAL_MINUTE_SECOND:
case INTERVAL_SECOND:
return SqlTypeName.MIN_INTERVAL_START_PRECISION;
default:
return RelDataType.PRECISION_NOT_SPECIFIED;
}
}

@Override public int getMaxNumericScale() {
return 19;
}
Expand All @@ -162,6 +259,10 @@ public abstract class RelDataTypeSystemImpl implements RelDataTypeSystem {
return 19;
}

@Override public int getMinNumericScale() {
return 0;
}

@Override public RoundingMode roundingMode() {
return RoundingMode.DOWN;
}
Expand Down
5 changes: 4 additions & 1 deletion core/src/main/java/org/apache/calcite/rex/RexBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1338,7 +1338,10 @@ protected RexLiteral makeLiteral(
case DECIMAL:
if (o != null && type.getScale() != RelDataType.SCALE_NOT_SPECIFIED) {
assert o instanceof BigDecimal;
o = ((BigDecimal) o).setScale(type.getScale(), RoundingMode.DOWN);
o = ((BigDecimal) o).setScale(type.getScale(), typeFactory.getTypeSystem().roundingMode());
if (type.getScale() < 0) {
o = new BigDecimal(((BigDecimal) o).toPlainString());
}
}
break;
default:
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/java/org/apache/calcite/runtime/CalciteResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -942,6 +942,12 @@ ExInst<CalciteException> illegalArgumentForTableFunctionCall(String a0,
@BaseMessage("Illegal arguments for 'MAP_VALUES' function: using a map with a null key is not allowed")
ExInst<CalciteException> illegalMapValuesWithNullKey();

@BaseMessage("DECIMAL precision {0,number,#} must be between 1 and {1,number,#}")
ExInst<CalciteException> invalidPrecisionForDecimalType(int precision, int maxPrecision);

@BaseMessage("DECIMAL scale {0,number,#} must be between {1,number,#} and {2,number,#}")
ExInst<CalciteException> invalidScaleForDecimalType(int scale, int minScale, int maxScale);

@BaseMessage("Illegal arguments: The length of the keys array {0,number,#} is not equal to the length of the values array {1,number,#} in MAP_FROM_ARRAYS function")
ExInst<CalciteException> illegalArgumentsInMapFromArraysFunc(int arg0, int arg1);

Expand Down
15 changes: 8 additions & 7 deletions core/src/main/java/org/apache/calcite/sql/SqlBasicTypeNameSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,16 +105,16 @@ public SqlBasicTypeNameSpec(
}

public SqlBasicTypeNameSpec(SqlTypeName typeName, SqlParserPos pos) {
this(typeName, -1, -1, null, pos);
this(typeName, RelDataType.PRECISION_NOT_SPECIFIED, RelDataType.SCALE_NOT_SPECIFIED, null, pos);
}

public SqlBasicTypeNameSpec(SqlTypeName typeName, int precision, SqlParserPos pos) {
this(typeName, precision, -1, null, pos);
this(typeName, precision, RelDataType.SCALE_NOT_SPECIFIED, null, pos);
}

public SqlBasicTypeNameSpec(SqlTypeName typeName, int precision,
String charSetName, SqlParserPos pos) {
this(typeName, precision, -1, charSetName, pos);
this(typeName, precision, RelDataType.SCALE_NOT_SPECIFIED, charSetName, pos);
}

public SqlBasicTypeNameSpec(SqlTypeName typeName, int precision,
Expand Down Expand Up @@ -167,11 +167,11 @@ public int getPrecision() {
writer.keyword(getTypeName().getSimple());
}

if (sqlTypeName.allowsPrec() && (precision >= 0)) {
if (sqlTypeName.allowsPrec() && (precision != RelDataType.PRECISION_NOT_SPECIFIED)) {
final SqlWriter.Frame frame =
writer.startList(SqlWriter.FrameTypeEnum.FUN_CALL, "(", ")");
writer.print(precision);
if (sqlTypeName.allowsScale() && (scale >= 0)) {
if (sqlTypeName.allowsScale() && (scale != RelDataType.SCALE_NOT_SPECIFIED)) {
writer.sep(",", true);
writer.print(scale);
}
Expand All @@ -198,10 +198,11 @@ public int getPrecision() {
// NOTE jvs 15-Jan-2009: earlier validation is supposed to
// have caught these, which is why it's OK for them
// to be assertions rather than user-level exceptions.
if ((precision >= 0) && (scale >= 0)) {
if ((precision != RelDataType.PRECISION_NOT_SPECIFIED)
&& (scale != RelDataType.SCALE_NOT_SPECIFIED)) {
assert sqlTypeName.allowsPrecScale(true, true);
type = typeFactory.createSqlType(sqlTypeName, precision, scale);
} else if (precision >= 0) {
} else if (precision != RelDataType.PRECISION_NOT_SPECIFIED) {
assert sqlTypeName.allowsPrecNoScale();
type = typeFactory.createSqlType(sqlTypeName, precision);
} else {
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/calcite/sql/SqlIntervalQualifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ public static int combineStartPrecisionPreservingDefault(

public int getFractionalSecondPrecision(RelDataTypeSystem typeSystem) {
if (fractionalSecondPrecision == RelDataType.PRECISION_NOT_SPECIFIED) {
return typeName().getDefaultScale();
return typeSystem.getDefaultScale(typeName());
} else {
return fractionalSecondPrecision;
}
Expand Down
Loading
Loading