Expression rewrite filter pushdown for dates #12056
Conversation
…ome other things first
… '2024-05-05'::DATE = ts::date cases are caught
|
|
||
| // add one day | ||
| auto date_t_copy = result.GetValue<duckdb::date_t>(); | ||
| date_t_copy.days += 1; |
There was a problem hiding this comment.
What happens if this overflows - can we check with DATE_MAX?
| if (!is_constant) { | ||
| // means the matchers are flipped, so we need to flip our bindings | ||
| // for some reason an extra binding is added in this case. | ||
| cast_constant = bindings[4].get().Copy(); |
There was a problem hiding this comment.
There are four matches being created:
- Comparison (top-level)
- Cast
- Bound column ref (child of cast)
- Constant
I think the bindings are in the order in which they are found, so the order is either comparison/cast/bound colref/constant or comparison/constant/cast/bound colref, but I'm not 100% sure.
| // other side is varchar to date? | ||
| auto right = make_uniq<CastExpressionMatcher>(); | ||
| right->type = make_uniq<TypeMatcherId>(LogicalTypeId::DATE); | ||
| right->matcher = make_uniq<ConstantExpressionMatcher>(); |
There was a problem hiding this comment.
This could be a FoldableConstantMatcher
Tmonster
force-pushed
the
expression_rewrite_filter_pushdown_for_dates
branch
from
e9a1e0b
to
37cc021
Compare
|
@Mytherin This is ready for another review. I added a |
| @@ -154,6 +154,22 @@ bool TryAddOperator::Operation(uint64_t left, uint64_t right, uint64_t &result) | |||
| return OverflowCheckedAddition::Operation<uint64_t, uint64_t>(left, right, result); | |||
| } | |||
|
|
|||
| template <> | |||
| bool TryAddOperator::Operation(date_t left, date_t right, date_t &result) { | |||
| if (!Value::IsFinite(left) || !Value::IsFinite(right)) { | |||
There was a problem hiding this comment.
Can we make AddOperator::Operation(date_t, int32_t) call this?
| @@ -154,6 +154,22 @@ bool TryAddOperator::Operation(uint64_t left, uint64_t right, uint64_t &result) | |||
| return OverflowCheckedAddition::Operation<uint64_t, uint64_t>(left, right, result); | |||
| } | |||
|
|
|||
| template <> | |||
| bool TryAddOperator::Operation(date_t left, date_t right, date_t &result) { | |||
There was a problem hiding this comment.
Can we turn right into an int32_t? Adding two dates together does not make much logical sense
src/common/types/interval.cpp
Outdated
| Date::Convert(left, year, month, day); | ||
| int32_t year_diff = right.months / Interval::MONTHS_PER_YEAR; | ||
| year += year_diff; | ||
| month += right.months - year_diff * Interval::MONTHS_PER_YEAR; |
There was a problem hiding this comment.
This is adding a lot of duplicate code - in general when we add the Try methods, we rewrite the regular Add methods to use the Try, i.e. Interval::Add becomes something like:
interval_t result;
if (!Interval::TryAdd(left, right, result)) {
throw OutOfRangeException("Date out of range");
}
return result;Could we either do that, or revert to extending the TryAddOperator for date_t/int32_t as before?
There was a problem hiding this comment.
Could we either do that, or revert to extending the
TryAddOperatorfordate_t/int32_tas before?
Since we are close to the release, I decided not to rewrite the regular add method, fearing that I could add more bugs in the process. I went with extending the TryAddOperator for date_t/int32_t
Merge pull request duckdb/duckdb#12056 from Tmonster/expression_rewrite_filter_pushdown_for_dates
Create a new expression rewriting rule so that comparisons between timestamps and dates can be pushed down.
TODO:
I don't quite understand the matching logic, currently the expressions can be matched using the unordered policy. This way I can catch both cases of
ts::DATE = '2024-05-06'::DATEand'2024-05-06'::DATE = ts::DATEFor some reason though, if the arguments are switched, an extra binding is pushed back the the bindings.Should fix https://github.com/duckdblabs/duckdb-internal/issues/1964