Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Update TO_DATE, TO_TIMESTAMP scalar functions to support LargeUtf8, Utf8View #12929

Merged
merged 6 commits into from
Oct 16, 2024

Conversation

Omega359
Copy link
Contributor

Which issue does this PR close?

Closes #12928

Rationale for this change

Update functions to support Utf8View and LargeUtf8

What changes are included in this PR?

Code, tests, benchmark

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) functions labels Oct 15, 2024
@Omega359 Omega359 marked this pull request as ready for review October 15, 2024 01:49
datafusion/functions/src/datetime/common.rs Outdated Show resolved Hide resolved
query P
SELECT to_timestamp(t.ts, '%Y-%m-%d %H/%M/%S%#z', '%+', '%s', '%d-%m-%Y %H:%M:%S%#z') from ts_utf8_data as t
Copy link
Member

Choose a reason for hiding this comment

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

Where did it go to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm. Incorrect refactor ... I changed the sql a few times here to condense the tests and it looks like I may have inadvertently changed this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it's now part of the sql starting at line 2242

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @Omega359 and @findepi -- this is looking nice now

format1_builder.append_value("%+");
format2_builder.append_value("%c");
format3_builder.append_value("%Y-%m-%d 00:00:00");
c.bench_function("to_timestamp_with_formats_utf8", |b| {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

datafusion/functions/src/datetime/common.rs Outdated Show resolved Hide resolved
@alamb
Copy link
Contributor

alamb commented Oct 15, 2024

I merged up from main and applied @findepi 's comment suggestion. Once the tests pass I think this one is good to go

@alamb alamb merged commit 435f959 into apache:main Oct 16, 2024
24 checks passed
@alamb
Copy link
Contributor

alamb commented Oct 16, 2024

🚀

@Omega359 Omega359 changed the title Update TO_DATE, TO_TIMESTAMP scalar functions to support LargeUtf8, Utf8View fix: Update TO_DATE, TO_TIMESTAMP scalar functions to support LargeUtf8, Utf8View Oct 16, 2024
@Omega359 Omega359 deleted the feature/timestamp_utf8view_12928 branch October 25, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
functions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update TO_DATE, TO_TIMESTAMP scalar functions to support LargeUtf8, Utf8View
3 participants