Add Date type to InPredicate by isadikov · Pull Request #4794 · facebookincubator/velox

isadikov · 2023-05-01T08:06:08Z

This PR adds support for Date types in InPredicate expression.

This allows to run queries like this:

SELECT * FROM table WHERE date in (DATE '2022-01-01', DATE '2022-02-01');
SELECT * FROM orders WHERE cast(orderdate as date) in (DATE '2000-06-30', DATE '2000-09-27')

Without this patch, the query above would fail with

VeloxRuntimeError: Scalar function in not registered with arguments: (DATE, ARRAY<DATE>)

Note: the fix itself is very small, most of the code changes are for unit tests because DuckDB test code did not handle date literals.

netlify · 2023-05-01T08:06:18Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`5cce89b`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/648a2b32f7c2300009060c81

isadikov · 2023-05-07T19:17:19Z

@kgpai It seems the changes in the PR #4461 misreport the signature changes. My PR adds a new signature and does not update the existing ones so the signature test should pass but it fails instead: https://app.circleci.com/pipelines/github/facebookincubator/velox/24232/workflows/cba6d1ef-e3ac-4fc9-a9f2-8c414a6a61e7/jobs/153843.

Can you have a look and follow up on this? Is there a way to override the failure?

mbasmanova · 2023-05-09T13:22:03Z

@kgpai Krishna, would you help take a look?

kgpai · 2023-05-09T15:02:55Z

Hi @isadikov , The error says that in has two implementations for (date, array(date)-> boolean . Going through your PR to see if thats the case.

kgpai · 2023-05-09T15:09:44Z

Give me a few hours to replicate this problem locally so I can debug this problem.

kgpai · 2023-05-09T18:08:43Z

I see these signatures on CircleCI for

"in": ["(date,array(date)) -> boolean", "(timestamp,array(timestamp)) -> boolean", "(varchar,array(varchar)) -> boolean", "(boolean,array(boolean)) -> boolean", "(real,array(real)) -> boolean", "(bigint,array(bigint)) -> boolean", "(double,array(double)) -> boolean", "(integer,array(integer)) -> boolean", "(smallint,array(smallint)) -> boolean", "(tinyint,array(tinyint)) -> boolean", "(tinyint,array(tinyint)) -> boolean", "(tinyint,array(unknown)) -> boolean", "(smallint,array(smallint)) -> boolean", "(smallint,array(unknown)) -> boolean", "(integer,array(integer)) -> boolean", "(integer,array(unknown)) -> boolean", "(bigint,array(bigint)) -> boolean", "(bigint,array(unknown)) -> boolean", "(varchar,array(varchar)) -> boolean", "(varchar,array(unknown)) -> boolean", "(varbinary,array(varbinary)) -> boolean", "(varbinary,array(unknown)) -> boolean", "(date,array(date)) -> boolean", "(date,array(unknown)) -> boolean"]

"(date,array(date)) -> boolean" is both at start and end , however I dint see this when I built this locally on my mac.
Digging more..

kgpai · 2023-05-09T18:29:02Z

I have identified the problem, I will send a PR asap which after landing you can rebase against to fix this.

isadikov · 2023-05-09T19:10:37Z

Great, thanks!

Summary: We need to namespace signatures that are imported so that we can differentiate between spark and presto signatures. Currently PR: #4794 has a problem where it considers the spark and presto signature for dates in `in predicate` to be similar. Pull Request resolved: #4883 Reviewed By: Yuhta Differential Revision: D45705225 Pulled By: kgpai fbshipit-source-id: 6392943e6af3fe9acfdb6cc3d1b911266e4d633a

kgpai · 2023-05-09T20:23:36Z

@isadikov Can you rebase against main and retry ?

isadikov · 2023-05-09T21:28:43Z

Yes, I will rebase, thanks for fixing the issue!

velox/duckdb/conversion/DuckParser.cpp

majetideepak

Thanks, @isadikov

isadikov · 2023-05-23T00:56:35Z

@mbasmanova Could you review this PR? Thanks!

Yuhta · 2023-05-26T19:40:14Z

velox/functions/prestosql/InPredicate.cpp

+  VELOX_USER_CHECK(
+      !values.empty(),
+      "IN predicate expects at least one non-null value in the in-list");
+  if (values.size() == 1) {


This is already handled in createBigintValues

Sure, I removed this part.

isadikov · 2023-05-30T21:19:06Z

@Yuhta I addressed your comments, could you review this PR? Thanks.

isadikov · 2023-06-07T20:55:28Z

@Yuhta Could you review this PR again? Thanks.

isadikov · 2023-06-12T20:32:25Z

@Yuhta Would you be able to take another look at the PR? Thanks.

Yuhta · 2023-06-15T17:14:45Z

We probably need to hold on this one until #5015 is merged

aditi-pandit · 2023-06-15T19:46:25Z

Hi @Yuhta : I'm fine with this going ahead of #5015 as it is blocking TPC-DS. I will merge these changes into that PR. Please go ahead with the merge.

facebook-github-bot · 2023-06-15T19:53:19Z

@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

aditi-pandit · 2023-06-15T19:51:42Z

velox/duckdb/conversion/DuckParser.cpp

+            ExpressionType::VALUE_CONSTANT) {
+          auto constExpr =
+              dynamic_cast<ConstantExpression*>(castExpr->child.get());
+          auto value =


Please check constExpr is non-null before de-referencing it in the next line. We should ensure that cast is not over any other expression here.

facebook-github-bot · 2023-06-16T00:20:06Z

@Yuhta merged this pull request in 24c20be.

Summary: We need to namespace signatures that are imported so that we can differentiate between spark and presto signatures. Currently PR: facebookincubator#4794 has a problem where it considers the spark and presto signature for dates in `in predicate` to be similar. Pull Request resolved: facebookincubator#4883 Reviewed By: Yuhta Differential Revision: D45705225 Pulled By: kgpai fbshipit-source-id: 6392943e6af3fe9acfdb6cc3d1b911266e4d633a

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 1, 2023

isadikov force-pushed the add_date_to_in branch 4 times, most recently from 99b89d9 to 0559610 Compare May 2, 2023 07:51

ethanyzhang mentioned this pull request May 2, 2023

TPC-DS Q83: Filter(MultiRange, deterministic, null not allowed): testInt64() is not supported prestodb/presto#19522

Closed

This was referenced May 9, 2023

[native] Handle Date type in toFilter function prestodb/presto#19593

Merged

Add support to check changes in function signatures #4461

Closed

kgpai mentioned this pull request May 9, 2023

Ensure signatures are namespaced. #4883

Closed

isadikov mentioned this pull request May 9, 2023

TPC-DS Q78: BaseVector.h:113 Check failed: dynamic_cast<const T*>(this) != nullptr prestodb/presto#19599

Closed

isadikov force-pushed the add_date_to_in branch 2 times, most recently from 4e24e59 to 08331dc Compare May 16, 2023 20:47

isadikov requested review from Yuhta, aditi-pandit and majetideepak May 16, 2023 20:54

isadikov force-pushed the add_date_to_in branch 3 times, most recently from 66d33a0 to b7d0a39 Compare May 17, 2023 06:54

majetideepak reviewed May 17, 2023

View reviewed changes

velox/duckdb/conversion/DuckParser.cpp Outdated Show resolved Hide resolved

isadikov force-pushed the add_date_to_in branch 2 times, most recently from 7eb5676 to e533fca Compare May 17, 2023 20:41

isadikov requested a review from majetideepak May 17, 2023 20:42

isadikov force-pushed the add_date_to_in branch from e533fca to 6c9a17e Compare May 18, 2023 06:07

majetideepak approved these changes May 18, 2023

View reviewed changes

majetideepak marked this pull request as ready for review May 18, 2023 07:06

isadikov force-pushed the add_date_to_in branch from 6c9a17e to 2970f63 Compare May 24, 2023 19:47

Yuhta reviewed May 26, 2023

View reviewed changes

isadikov force-pushed the add_date_to_in branch from 2970f63 to 594864b Compare May 28, 2023 19:58

isadikov force-pushed the add_date_to_in branch 2 times, most recently from 3fa1498 to 9b78a5f Compare June 4, 2023 21:46

Yuhta self-requested a review June 7, 2023 22:46

isadikov force-pushed the add_date_to_in branch from 9b78a5f to 0a3d206 Compare June 11, 2023 19:39

add date to in predicate

5cce89b

isadikov force-pushed the add_date_to_in branch from 0a3d206 to 5cce89b Compare June 14, 2023 21:03

aditi-pandit reviewed Jun 15, 2023

View reviewed changes

facebook-github-bot closed this in 24c20be Jun 16, 2023

facebook-github-bot added the Merged label Jun 16, 2023

Conversation

isadikov commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

isadikov commented May 7, 2023

Uh oh!

mbasmanova commented May 9, 2023

Uh oh!

kgpai commented May 9, 2023

Uh oh!

kgpai commented May 9, 2023

Uh oh!

kgpai commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kgpai commented May 9, 2023

Uh oh!

isadikov commented May 9, 2023

Uh oh!

kgpai commented May 9, 2023

Uh oh!

isadikov commented May 9, 2023

Uh oh!

Uh oh!

majetideepak left a comment

Choose a reason for hiding this comment

Uh oh!

isadikov commented May 23, 2023

Uh oh!

Yuhta May 26, 2023

Choose a reason for hiding this comment

Uh oh!

isadikov May 28, 2023

Choose a reason for hiding this comment

Uh oh!

isadikov commented May 30, 2023

Uh oh!

isadikov commented Jun 7, 2023

Uh oh!

isadikov commented Jun 12, 2023

Uh oh!

Yuhta commented Jun 15, 2023

Uh oh!

aditi-pandit commented Jun 15, 2023

Uh oh!

facebook-github-bot commented Jun 15, 2023

Uh oh!

aditi-pandit Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

isadikov commented May 1, 2023 •

edited

Loading

netlify bot commented May 1, 2023 •

edited

Loading

kgpai commented May 9, 2023 •

edited

Loading