gortiz commented on code in PR #15263:
URL: https://github.com/apache/pinot/pull/15263#discussion_r1998582208
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -62,16 +62,36 @@ public boolean shouldConvertRaggedUnionTypesToVarying() {
return true;
}
+ @Override
+ public int getMaxScale(SqlTypeName typeName) {
+ return typeName == SqlTypeName.DECIMAL ? MAX_DECIMAL_SCALE :
super.getMaxScale(typeName);
+ }
+
@Override
public int getMaxNumericScale() {
return MAX_DECIMAL_SCALE;
}
+ @Override
+ public int getDefaultScale(SqlTypeName typeName) {
+ return typeName == SqlTypeName.DECIMAL ? MAX_DECIMAL_SCALE :
super.getDefaultScale(typeName);
+ }
+
+ @Override
+ public int getMaxPrecision(SqlTypeName typeName) {
+ return typeName == SqlTypeName.DECIMAL ? MAX_DECIMAL_PRECISION :
super.getMaxPrecision(typeName);
+ }
+
@Override
public int getMaxNumericPrecision() {
return MAX_DECIMAL_PRECISION;
}
+ @Override
+ public int getDefaultPrecision(SqlTypeName typeName) {
+ return typeName == SqlTypeName.DECIMAL ? MAX_DECIMAL_PRECISION :
super.getDefaultPrecision(typeName);
Review Comment:
I don't think this is correct. IICU this means our default is `DECIMAL(1000,
2000)`, which means use a 1000 digits as total number of digits (both before
and after the decimal point) and 1000 digits to the right of the decimal point.
Therefore we could only store values in the range of (-1, 1).
Also, I don't know the implications these digits may have, but in BigDecimal
scales from 1 to 10 (or -1 to -10, but we don't care about them) are more
efficient than the others
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java:
##########
@@ -244,10 +246,10 @@ private static RexExpression.Literal
fromRexLiteralValue(ColumnDataType dataType
value = ((BigDecimal) value).longValue();
break;
case FLOAT:
- value = ((BigDecimal) value).floatValue();
+ value = ((Double) value).floatValue();
break;
case DOUBLE:
- value = ((BigDecimal) value).doubleValue();
+ value = value instanceof BigDecimal ? ((BigDecimal)
value).doubleValue() : value;
Review Comment:
nit: Couldn't we just cast it as Number and call `doubleValue`?
##########
pinot-query-planner/src/test/resources/queries/JoinPlans.json:
##########
@@ -405,10 +405,10 @@
"\nLogicalProject(col1=[$0])",
"\n LogicalJoin(condition=[AND(=($0, $3), =($1, $4), >($2, $5))],
joinType=[inner])",
"\n PinotLogicalExchange(distribution=[hash[0, 1]])",
- "\n LogicalProject(col1=[$0], col2=[$1], col4=[$3])",
+ "\n LogicalProject(col1=[$0], col2=[$1],
EXPR$0=[CAST($3):DECIMAL(1000, 982) NOT NULL])",
Review Comment:
Do we know why the precision is 982?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
