kasakrisz commented on code in PR #3811:
URL: https://github.com/apache/hive/pull/3811#discussion_r1044476017
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java:
##########
@@ -1261,6 +1264,8 @@ static FilterType fromType(String colTypeStr) {
return FilterType.Date;
} else if (ColumnType.IntegralTypes.contains(colTypeStr)) {
return FilterType.Integral;
+ } else if (colTypeStr.equals(ColumnType.TIMESTAMP_TYPE_NAME)) {
Review Comment:
It would be nice to get rid of these if ... else if ... statements.
How about declaring a `ColumnType` field in the `FilterType` enum and pass
the corresponding `ColumnType` in the constructor if each enum constant.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/parser/ExpressionTree.java:
##########
@@ -456,19 +457,22 @@ private String getJdoFilterPushdownParam(int partColIndex,
boolean isIntegralSupported = canPushDownIntegral &&
canJdoUseStringsWithIntegral();
String colType = partitionKeys.get(partColIndex).getType();
// Can only support partitions whose types are string, or maybe integers
- // Date data type value is considered as string hence pushing down to
JDO.
+ // Date/Timestamp data type value is considered as string hence pushing
down to JDO.
if (!colType.equals(ColumnType.STRING_TYPE_NAME) &&
!colType.equals(ColumnType.DATE_TYPE_NAME)
+ && !colType.equals(ColumnType.TIMESTAMP_TYPE_NAME)
Review Comment:
Can `colType` be `null` here? Does case sensibility matters?
Please consider swapping the operands like
```
ColumnType.TIMESTAMP_TYPE_NAME.equalsIgnoreCase(colType)
```
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java:
##########
@@ -1309,10 +1316,19 @@ public void visit(LeafNode node) throws MetaException {
}
}
+ if (colType == FilterType.Timestamp && valType == FilterType.String) {
Review Comment:
Moving forward the previous idea:
Instead if the `FilterType` enum a `Filter` interface or abstract class can
be used and movie these logic to corresponding subclasses like `StringFilter`,
`DateFiler` etc
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/parser/Filter.g:
##########
@@ -70,6 +84,19 @@ import java.util.regex.Pattern;
}
}
+ public static java.sql.Timestamp ExtractTimestamp (String input) {
Review Comment:
Please start the method name with lowercase letter. `ExtractDate` is also
wrong.
--
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]
