hudi-agent commented on code in PR #20294:
URL: https://github.com/apache/hudi/pull/20294#discussion_r4257215474
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/source/ExpressionPredicates.java:
##########
@@ -653,8 +659,26 @@ private static FilterPredicate
toParquetPredicate(FunctionDefinition functionDef
case DATE:
case TIME_WITHOUT_TIME_ZONE:
return predicateSupportsLtGt(functionDefinition,
intColumn(columnName), (Integer) literal);
- case BIGINT:
case TIMESTAMP_WITHOUT_TIME_ZONE:
Review Comment:
🤖 nit: could you move the TIMESTAMP_WITHOUT_TIME_ZONE conversion into a
small helper (e.g. `toTimestampPredicate`)? It returns null in several places
and makes `toParquetPredicate` hard to scan.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/source/ExpressionPredicates.java:
##########
@@ -231,7 +235,8 @@ public ColumnPredicate
bindValueLiteral(ValueLiteralExpression valueLiteral) {
@Override
public FilterPredicate filter() {
- Serializable convertedLiteral =
ImplicitTypeConverter.convertImplicitly(literalType, literal);
+ Serializable convertedLiteral = literalType instanceof TimestampType
Review Comment:
🤖 non-blocking: Because of this bypass, no main code reaches the
`TIMESTAMP_WITHOUT_TIME_ZONE` branch in `ImplicitTypeConverter` any more, yet
it still truncates to UTC epoch millis, and a String literal on a timestamp
field (the case HUDI-7311 added) silently stops pushing down. Would it make
sense to have that branch return a `LocalDateTime` (parsing strings) and drop
the `instanceof TimestampType` special case here?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/test/java/org/apache/hudi/source/TestExpressionPredicates.java:
##########
@@ -195,6 +199,85 @@ public void
testDisablePredicatesPushDownForUnsupportedType() {
assertNull(Not.getInstance().bindPredicate(greaterThanPredicate).filter(),
"Decimal type push down is unsupported, so we expect null");
}
+ @ParameterizedTest
+ @ValueSource(ints = {0, 1, 2, 4, 5, 7, 8, 9})
+ void testDisableTimestampPredicatePushDown(int precision) {
+ FieldReferenceExpression field = new FieldReferenceExpression("ts",
DataTypes.TIMESTAMP(precision), 0, 0);
+ ValueLiteralExpression literal = new
ValueLiteralExpression(LocalDateTime.parse("1970-01-01T00:00:01.000001"));
+ BuiltInFunctionDefinition[] functions = {
+ BuiltInFunctionDefinitions.EQUALS,
BuiltInFunctionDefinitions.NOT_EQUALS,
+ BuiltInFunctionDefinitions.LESS_THAN,
BuiltInFunctionDefinitions.GREATER_THAN,
+ BuiltInFunctionDefinitions.LESS_THAN_OR_EQUAL,
BuiltInFunctionDefinitions.GREATER_THAN_OR_EQUAL
+ };
Review Comment:
🤖 nit: the six-function array is duplicated in two tests; could you extract
it into a shared constant or helper?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
