Skip to content

Convert DATE to logical type#5015

Closed
aditi-pandit wants to merge 1 commit intomainfrom
date_refactor
Closed

Convert DATE to logical type#5015
aditi-pandit wants to merge 1 commit intomainfrom
date_refactor

Conversation

@aditi-pandit
Copy link
Collaborator

@aditi-pandit aditi-pandit commented May 19, 2023

No description provided.

@netlify
Copy link

netlify bot commented May 19, 2023 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 2eca707
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6494d713cf1be500081f1031

@aditi-pandit aditi-pandit marked this pull request as draft May 19, 2023 18:53
@facebook-github-bot 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 19, 2023
@aditi-pandit aditi-pandit force-pushed the date_refactor branch 12 times, most recently from e0f27e4 to d52d8b4 Compare May 26, 2023 18:15
@aditi-pandit aditi-pandit force-pushed the date_refactor branch 13 times, most recently from 18d80b8 to 3bbb9c2 Compare May 31, 2023 06:19
@aditi-pandit aditi-pandit requested a review from mbasmanova May 31, 2023 18:30
@aditi-pandit aditi-pandit marked this pull request as ready for review May 31, 2023 18:30
@Yuhta Yuhta mentioned this pull request Jun 15, 2023
Closed
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 15, 2023

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

bikramSingh91
bikramSingh91 reviewed Jun 15, 2023
View reviewed changes
velox/expression/CastExpr.cpp
} else {
castFromOperator_->castFrom(input, context, rows, toType, result);
}
} else if (fromType->isDate()) {
Copy link
Contributor

@bikramSingh91 bikramSingh91 Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious, does the casting to/from "Interval day time" work correctly?

Copy link
Collaborator Author

@aditi-pandit aditi-pandit Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now "interval day time" works just like an integer. In general, I don't think "interval day time" can be cast to any type. Though we should double check. The most type-safe behavior would be to error out in CastExpr if type is interval day time.

velox/expression/CastExpr.cpp
(*castResult).clearNulls(rows);
auto* resultFlatVector = castResult->as<FlatVector<int32_t>>();
switch (fromType->kind()) {
case TypeKind::VARCHAR: {
Copy link
Contributor

@bikramSingh91 bikramSingh91 Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to support VARBINARY too?
cc: @kagamiori

Copy link
Collaborator Author

@aditi-pandit aditi-pandit Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't supported in the past. This PR is just for refactoring. We can create an issue and add support subsequently.

@aditi-pandit aditi-pandit force-pushed the date_refactor branch 2 times, most recently from 26b76c9 to 38c852d Compare June 20, 2023 23:29
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 21, 2023

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

@aditi-pandit
Copy link
Collaborator Author

aditi-pandit commented Jun 21, 2023

Thanks @bikramSingh91 for importing these changes. There is a FB Linter error. Is there anything I can help with ?

@bikramSingh91
Copy link
Contributor

bikramSingh91 commented Jun 22, 2023

Thanks @bikramSingh91 for importing these changes. There is a FB Linter error. Is there anything I can help with ?

Thanks for checking. I fixed the internal lint errors, but looks like this is being used in prestissimo and some other places internally. So I'll have to spend some time fixing those and ensuring nothing breaks before moving ahead with this.

aditi-pandit pushed a commit to prestodb/presto that referenced this pull request Jun 22, 2023
[native] Fixes for removing TypeKind::DATE
0b802a5 
Velox Date is changed to a LogicalType instead of
a first-class TypeKind. See
facebookincubator/velox#5015

Fix usages of the TypeKind::DATE in Prestissimo code inline
with the new implementation.
@aditi-pandit aditi-pandit mentioned this pull request Jun 22, 2023
Closed
@aditi-pandit
Copy link
Collaborator Author

aditi-pandit commented Jun 22, 2023

Thanks @bikramSingh91 for importing these changes. There is a FB Linter error. Is there anything I can help with ?

Thanks for checking. I fixed the internal lint errors, but looks like this is being used in prestissimo and some other places internally. So I'll have to spend some time fixing those and ensuring nothing breaks before moving ahead with this.

Yes, Prestissimo changes are needed for advancing Velox version. I had prestodb/presto#19935 in mind. Please let me know if you need any help.

@aditi-pandit
Copy link
Collaborator Author

aditi-pandit commented Jun 23, 2023

@bikramSingh91 , @Yuhta : Rebased this code since there were multiple conflicts reported. Please let me know if you need anything else from me.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 23, 2023

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

@bikramSingh91 bikramSingh91 mentioned this pull request Jul 6, 2023
Merged
bikramSingh91 pushed a commit to bikramSingh91/presto that referenced this pull request Jul 6, 2023
@aditi-pandit @facebook-github-bot
Convert DATE to logical type (prestodb#5015)
01a2e34 
Summary: Pull Request resolved: facebookincubator/velox#5015

Reviewed By: Yuhta

Differential Revision: D46781864

Pulled By: bikramSingh91

fbshipit-source-id: cd8ef05ede44131f361a759025f14fde431addba
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 6, 2023

@bikramSingh91 merged this pull request in e480f5c.

@conbench-facebook
Copy link

conbench-facebook bot commented Jul 6, 2023

Conbench analyzed the 1 benchmark run on commit e480f5c0.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

mshang816 pushed a commit to prestodb/presto that referenced this pull request Jul 6, 2023
@aditi-pandit @mshang816
Convert DATE to logical type (#5015)
507ca0c 
Summary: Pull Request resolved: facebookincubator/velox#5015

Reviewed By: Yuhta

Differential Revision: D46781864

Pulled By: bikramSingh91

fbshipit-source-id: cd8ef05ede44131f361a759025f14fde431addba
@aditi-pandit
Copy link
Collaborator Author

aditi-pandit commented Jul 8, 2023

Thanks @bikramSingh91 for merging this. Appreciate it.

@aditi-pandit aditi-pandit deleted the date_refactor branch July 8, 2023 19:38
wypb pushed a commit to wypb/presto that referenced this pull request Dec 22, 2023
@aditi-pandit @mshang816
Convert DATE to logical type (prestodb#5015)
5a8aba3 
Summary: Pull Request resolved: facebookincubator/velox#5015

Reviewed By: Yuhta

Differential Revision: D46781864

Pulled By: bikramSingh91

fbshipit-source-id: cd8ef05ede44131f361a759025f14fde431addba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@majetideepak majetideepak majetideepak left review comments

@bikramSingh91 bikramSingh91 bikramSingh91 left review comments

@mbasmanova mbasmanova mbasmanova left review comments

@Yuhta Yuhta Yuhta approved these changes

@laithsakka laithsakka Awaiting requested review from laithsakka

+1 more reviewer

@jiangjiguang jiangjiguang jiangjiguang left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@aditi-pandit @majetideepak @bikramSingh91 @facebook-github-bot @Yuhta @jiangjiguang @mbasmanova