[multistage] Support TIMESTAMP type and date ops functions #11350
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11350 +/- ##
=========================================
Coverage 61.46% 61.46%
- Complexity 6512 6513 +1
=========================================
Files 2234 2234
Lines 120155 120191 +36
Branches 18235 18244 +9
=========================================
+ Hits 73854 73880 +26
- Misses 40888 40890 +2
- Partials 5413 5421 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 10 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
xiangfu0
force-pushed
the
fixing-timstamp-type
branch
3 times, most recently
from
cc9c0b9 to
d94bc25
Compare
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java
Show resolved
Hide resolved
| @@ -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) { | |||
There was a problem hiding this 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
| @@ -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) { | |||
There was a problem hiding this comment.
I feel this applies to all data types
There was a problem hiding this comment.
For other data types, only BIG_DECIMAL is the inferable type. So just need to add support for that.
| @@ -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) { | |||
There was a problem hiding this 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
There was a problem hiding this comment.
Certain non strong typed functions in v2 might return long value then in datablock build, we need to do the conversion.
| @@ -353,6 +353,9 @@ public Serializable convert(Object value) { | |||
| case BOOLEAN: | |||
| return ((Number) value).intValue() == 1; | |||
| case TIMESTAMP: | |||
| if (value instanceof Timestamp) { | |||
There was a problem hiding this comment.
Does this still apply? Ideally we should always have internal format value. If we break that contract, BOOLEAN will also break
| @@ -124,6 +124,10 @@ public enum TransformFunctionType { | |||
| SqlTypeFamily.CHARACTER), | |||
| ordinal -> ordinal > 1)), | |||
|
|
|||
| FROMDATETIME("fromDateTime", ReturnTypes.TIMESTAMP_NULLABLE, | |||
There was a problem hiding this comment.
Please add a TODO to auto generate signature for scalar function. We should not manually add this for scalar function
[multistage] Convert TIMESTAMP type value at reduce phase
xiangfu0
force-pushed
the
fixing-timstamp-type
branch
from
d94bc25 to
0c02242
Compare
xiangfu0
changed the title
xiangfu0
force-pushed
the
fixing-timstamp-type
branch
2 times, most recently
from
0b5b0f7 to
284ede5
Compare
xiangfu0
force-pushed
the
fixing-timstamp-type
branch
from
284ede5 to
a386ac2
Compare
fromDateTimetoDateTimetimestampAddtimestampDiffyearyearOfWeekmonthOfYearweekOfYeardayOfYeardayOfMonthdayOfWeekhourminutesecondmillisecondNote
quarteris not supported in v2 yet.Before:
After:
