mihaibudiu commented on code in PR #5243:
URL: https://github.com/apache/calcite/pull/5243#discussion_r4090268184
##########
druid/src/main/java/org/apache/calcite/adapter/druid/DruidDateTimeUtils.java:
##########
@@ -126,9 +126,11 @@ protected static List<Interval> toInterval(
for (RexNode child : call.getOperands()) {
List<Range<Long>> extracted =
extractRanges(child, withNot);
- if (extracted != null) {
- intervals.addAll(extracted);
+ if (extracted == null) {
+ // Every disjunct must be represented to avoid excluding matching
rows.
+ return null;
Review Comment:
This function has no javadoc, so it's hard to guess what "null" means.
It's hard even to guess what the returned result is supposed to be.
Maybe you can improve it a bit?
##########
druid/src/test/java/org/apache/calcite/adapter/druid/DruidQueryFilterTest.java:
##########
@@ -138,6 +156,42 @@ class DruidQueryFilterTest {
+ "\"ordering\":\"lexicographic\"}"));
}
+ @Test void testOrWithExtractRetainsFilter() {
+ final Fixture f = new Fixture();
+ final HepPlanner planner =
+ new HepPlanner(new
HepProgramBuilder().addRuleInstance(DruidRules.FILTER).build(),
+ Contexts.of(new CalciteConnectionConfigImpl(new Properties())));
+ final RelOptCluster cluster = RelOptCluster.create(planner, f.rexBuilder);
+ final RelDataType rowType = f.typeFactory.builder()
+ .add("timestamp", SqlTypeName.TIMESTAMP)
+ .build();
+ final DruidTable table =
+ new DruidTable(Mockito.mock(DruidSchema.class), "events", factory ->
rowType,
+ ImmutableSet.of(), "timestamp", null, null, null);
+ final RelOptTable relOptTable =
+ RelOptTableImpl.create(null, rowType, ImmutableList.of("events"),
table,
+ clazz -> null);
+ final RelNode scan = LogicalTableScan.create(cluster, relOptTable,
ImmutableList.of());
+ final DruidQuery query =
+ DruidQuery.create(cluster,
cluster.traitSet().plus(BindableConvention.INSTANCE),
+ relOptTable, table, ImmutableList.of(scan));
+ final RexNode timestamp = f.rexBuilder.makeInputRef(scan, 0);
+ final RexNode condition =
+ f.rexBuilder.makeCall(
+ SqlStdOperatorTable.OR,
f.rexBuilder.makeCall(SqlStdOperatorTable.LESS_THAN, timestamp,
+ f.rexBuilder.makeTimestampLiteral(new TimestampString("2020-01-01
00:00:00"), 0)),
+ f.rexBuilder.makeCall(SqlStdOperatorTable.EQUALS,
+ f.rexBuilder.makeCall(SqlStdOperatorTable.EXTRACT,
+ f.rexBuilder.makeFlag(TimeUnitRange.DAY), timestamp),
+ f.rexBuilder.makeExactLiteral(BigDecimal.valueOf(15))));
+ planner.setRoot(LogicalFilter.create(query, condition));
Review Comment:
inferring the generated code is not easy, maybe you can help by adding some
comments showing the equivalent SQL for the various expressions generated
##########
druid/src/test/java/org/apache/calcite/test/DruidDateRangeRulesTest.java:
##########
@@ -59,6 +60,23 @@ class DruidDateRangeRulesTest {
is("[2011-01-01T00:00:00.000Z/2012-02-02T00:00:00.001Z]"));
}
+ @Test void testOrWithUnextractableRange() {
+ final Fixture2 f = new Fixture2();
+ final RexNode range = f.lt(f.ts, f.timestampLiteral(2020,
Calendar.JANUARY, 1));
+ final RexNode extract = f.eq(f.exDay, f.literal(15));
+ assertThat(DruidDateTimeUtils.createInterval(f.or(range, extract)),
nullValue());
Review Comment:
createInterval also does not say what a "null" result means, maybe you can
improve its documentation
--
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]
