[native] Handle Date type in toFilter function

[isadikov]

This PR fixes TPC-DS Q83 error:

VeloxUserError: Filter(MultiRange, deterministic, null not allowed): testInt64() is not supported

In query 83, the date filter is constructed as a generic MultiRange filter instead of BigintMultiRange as dates are converted into int64_t. MultiRange filter does not support testInt64 method and crashes. Instead, we should handle dates the same way we handle integers.

This is complimentary to facebookincubator/velox#4794.

I added a test that verifies this change.

== NO RELEASE NOTE ==

Fixes #19522.

[isadikov]

cc @majetideepak @aditi-pandit to review. Thanks!

[majetideepak]

@isadikov can we add a test?

[majetideepak]

Can we use the if block above that handles integers?

[isadikov]

I suppose we can, however, there would still be a if-else statement because we need to use dateRangeToFilter instead of bigintRangeToFilter.

[majetideepak]

I missed the dateRangeToFilter difference. This is fine. We can remove this when Date becomes a logical type.

[isadikov]

The test part is mentioned in the description. I could not reproduce this issue in a test so I decided to include the fix only. We will add TPC-DS queries later so this will be covered. For now, I can only manually verify the patch.

[majetideepak]

I could not reproduce this issue in a test

Why can we not reproduce this in a test? Why only in TPC-DS Q83? Is this dependent on Decimal Types?

[isadikov]

I added a test to verify the fix, the test fails without the fix and passes with the fix.

[majetideepak]

@isadikov can you remove the "Advance Velox" bits?

[isadikov]

Oh, sorry about that, it was unintentional. I updated the PR.

[aditi-pandit]

Do we require to do this integer range combination logic for date filters ? Ideally, date range is from 01-01-0001 to 12-31-9999, and the complex logic in combineIntegerRanges might be overkill for dates.

[isadikov]

That is true, having a complicated combination logic is not required for date filters because, as you mentioned, it is a much smaller range. I suppose I just wanted to keep this similar to integer ranges and avoid implementing combineDateRanges function since combineIntegerRanges is a superset.

Would you like to have a separate combineDateRanges function for date ranges?

[aditi-pandit]

Can the date ranges just be retained as originally specified without a combine function ? It should be fine to keep the original ranges just so I think.

[majetideepak]

This code block merges multiple ranges in a domain into a single one. I think we have to combine here.

[aditi-pandit]

combineIntegerRanges is a misnomer in the sense its not really combining range filters when there is overlap. It is doing :
i) Creating a values filter if all the filters are checking a single value
ii) Creating NegatedBigIntRange filters if there is only a small big int interval is that is omitted by the range filters.
iii) Else its returning a MultiRange Filter with the individual ranges.

For dates specifically the negated filters logic will not get triggered. For the most part only MultiRangeFilter will be returned. So this function is not really useful for Date filters.

In general, I feel its better to return a BigIntMultiRange filter here and then add optimizations for Date later as needed.

[isadikov]

I can follow up on streamlining the logic for Date type in a separate PR once this PR is merged if that works.

[majetideepak]

@aditi-pandit thanks for the clarification.

[isadikov]

I will update, thanks!

[isadikov]

@aditi-pandit @majetideepak Could you review this PR also? I had to make a few changes including CI build to make the test pass - we need Parquet support for it. Thanks!

[isadikov]

Strange, my first rebase removed the tests from the patch. I had to apply the changes manually and recreate the commit to have all of the original changes in the PR. All should be good now.

[isadikov]

@aditi-pandit Could you review this PR? Thanks.

[aditi-pandit]

Thanks @isadikov. This looks good.