zabetak commented on a change in pull request #2672:
URL: https://github.com/apache/calcite/pull/2672#discussion_r778965499
##########
File path: druid/src/main/java/org/apache/calcite/adapter/druid/DruidRules.java
##########
@@ -854,4 +851,33 @@ protected DruidSortRule(DruidSortRuleConfig config) {
}
}
}
+
+ /**
+ * Describes some filters to be applied to Druid.
+ */
+ private static class DruidFilters {
+
+ private final List<RexNode> timeRangeNodes;
+ private final List<RexNode> pushableNodes;
+ private final List<RexNode> nonPushableNodes;
+
+ private DruidFilters(List<RexNode> timeRangeNodes, List<RexNode>
pushableNodes,
+ List<RexNode> nonPushableNodes) {
+ this.timeRangeNodes = timeRangeNodes;
+ this.pushableNodes = pushableNodes;
+ this.nonPushableNodes = nonPushableNodes;
+ }
+
+ public List<RexNode> getTimeRangeNodes() {
+ return timeRangeNodes;
+ }
+
+ public List<RexNode> getPushableNodes() {
+ return pushableNodes;
+ }
+
+ public List<RexNode> getNonPushableNodes() {
+ return nonPushableNodes;
+ }
+ }
Review comment:
Calcite makes use of the
[immutables](https://github.com/immutables/immutables) library. Wouldn't be
better to use the immutable annotations to create the POJO?
##########
File path: file/src/main/java/org/apache/calcite/adapter/file/CsvEnumerator.java
##########
@@ -68,18 +69,16 @@
private final RowConverter<E> rowConverter;
private @Nullable E current;
- private static final FastDateFormat TIME_FORMAT_DATE;
- private static final FastDateFormat TIME_FORMAT_TIME;
- private static final FastDateFormat TIME_FORMAT_TIMESTAMP;
+ private static final DateTimeFormatter TIME_FORMAT_DATE;
+ private static final DateTimeFormatter TIME_FORMAT_TIME;
+ private static final DateTimeFormatter TIME_FORMAT_TIMESTAMP;
Review comment:
It is a good idea to use `DateTimeFormatter` and we should do it. We
should be a bit careful though cause every time there is a change in
Date/Time/Timestamp parsing there are problems (many times backward
compatibility) that go unnoticed.
Would it be possible to add some tests for dates/timestamps in the range
`[0000-01-01 ,9999-12-29]` (a snippet generating random timestamps can be found
[here](https://github.com/apache/hive/blob/3c9ea5d8d6ecc0d3749e22b28bfb7690dd3f1be1/ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestParquetTimestampsHive2Compatibility.java#L118))
and check/document the differences with the old formatter. It's a bit
unfortunate that we don't have such tests already.
--
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]
