Skip to content

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 Changes to functions implementation labels Oct 15, 2024
@Omega359 Omega359 marked this pull request as ready for review October 15, 2024 01:49

# verify timestamp data using tables with formatting options where one of the formats is invalid
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.

👍

@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 Changes to functions implementation 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