Jackie-Jiang commented on code in PR #11350:
URL: https://github.com/apache/pinot/pull/11350#discussion_r1296815811
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -416,8 +419,14 @@ public Serializable convertAndFormat(Object value) {
case BIG_DECIMAL:
return (BigDecimal) value;
case BOOLEAN:
+ if (value instanceof Boolean) {
+ return (boolean) value;
Review Comment:
(minor)
```suggestion
return value;
```
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/parser/CalciteRexExpressionParser.java:
##########
@@ -194,6 +196,10 @@ public static Expression toExpression(RexExpression
rexNode, PinotQuery pinotQue
private static Expression rexLiteralToExpression(RexExpression.Literal
rexLiteral) {
// TODO: currently literals are encoded as strings for V1, remove this and
use directly literal type when it
// supports strong-type in V1.
+ if (rexLiteral.getDataType() == FieldSpec.DataType.TIMESTAMP
+ && rexLiteral.getValue() instanceof GregorianCalendar) {
Review Comment:
(minor) Suggest removing the `rexLiteral.getValue() instanceof
GregorianCalendar` so that if it returns unexpected value we can get exception
instead of a completely wrong literal
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java:
##########
@@ -121,6 +121,10 @@ public void init(List<TransformFunction> arguments,
Map<String, ColumnContext> c
parameterTypes[i].convert(literalTransformFunction.getBigDecimalLiteral(),
PinotDataType.BIG_DECIMAL);
break;
case TIMESTAMP:
+ if (parameterTypes[i] == PinotDataType.STRING) {
Review Comment:
I feel this applies to all data types
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -151,7 +151,11 @@ public static RowDataBlock buildFromRows(List<Object[]>
rows, DataSchema dataSch
byteBuffer.putInt(((Boolean) value) ? 1 : 0);
break;
case TIMESTAMP:
- byteBuffer.putLong(((Timestamp) value).getTime());
+ if (value instanceof Long) {
Review Comment:
Are we going to hit both? If so, can you add some java doc explaining when
will we hit both? If we can hit both, then `BOOLEAN` will also break
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -353,6 +353,9 @@ public Serializable convert(Object value) {
case BOOLEAN:
return ((Number) value).intValue() == 1;
case TIMESTAMP:
+ if (value instanceof Timestamp) {
Review Comment:
Does this still apply? Ideally we should always have internal format value.
If we break that contract, `BOOLEAN` will also break
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -124,6 +124,10 @@ public enum TransformFunctionType {
SqlTypeFamily.CHARACTER),
ordinal -> ordinal > 1)),
+ FROMDATETIME("fromDateTime", ReturnTypes.TIMESTAMP_NULLABLE,
Review Comment:
Please add a TODO to auto generate signature for scalar function. We should
not manually add this for scalar function
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
