Skip to content

[native] velox::Date and TypeKind::DATE will go away. Prepare for this.#19975

Open
spershin wants to merge 1 commit intoprestodb:masterfrom
spershin:PrepareForVeloxDateRefactor
Open

[native] velox::Date and TypeKind::DATE will go away. Prepare for this.#19975
spershin wants to merge 1 commit intoprestodb:masterfrom
spershin:PrepareForVeloxDateRefactor

Conversation

@spershin
Copy link
Contributor

== NO RELEASE NOTE ==

@spershin spershin requested a review from a team as a code owner June 23, 2023 20:34
@aditi-pandit
Copy link
Contributor

aditi-pandit commented Jun 28, 2023

@spershin : I had #19935 also to handle removing TypeKind::DATE. The intention was to put it along with the Advance Velox version PR to keep the changes isolated.

But it might be easier to move the Presto code to
i) Remove all uses to TypeKind::DATE and use type->isDate() along with the integer mappings (covered in this PR)
ii) Then make another set of fixes to remove unneeded type->isDate() calls. (PR # 19935)

We can go ahead with this PR and I'll change #19935 to do post-Velox merge changes.

}

if (type->kind() == TypeKind::DATE) {
if (type->isDate()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This block should be moved at line 642 before above conditional statement for TypeKind::INTEGER (see https://github.com/prestodb/presto/pull/19935/files#diff-b596594147f956ff5aadb512a14f1538d5975c551dad6ba773ac266762187734).

Without that change the Velox PR will start using the above conditional block for TypeKind::INTEGER since it will map date to TypeKind::INTEGER and that will take precedence. Instead the date related code should take precedence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants