Skip to content

ARROW-12540: [C++] Implementing casting support from date32/date64 to uft8/large_utf8#10870

Closed
rommelDB wants to merge 4 commits into
apache:masterfrom
rommelDB:12540-implement-cast-from-date-to-string
Closed

ARROW-12540: [C++] Implementing casting support from date32/date64 to uft8/large_utf8#10870
rommelDB wants to merge 4 commits into
apache:masterfrom
rommelDB:12540-implement-cast-from-date-to-string

Conversation

@rommelDB

@rommelDB rommelDB commented Aug 4, 2021

Copy link
Copy Markdown
Contributor

Changes:

  • Implemented casting support from date32/date64 to uft8/large_utf8
  • Added unit tests for converting from date32/date64 to uft8/large_utf8.

…nted the casting support from date32/date64 to uft8/large_utf8
@github-actions

github-actions Bot commented Aug 4, 2021

Copy link
Copy Markdown

@rommelDB

rommelDB commented Aug 4, 2021

Copy link
Copy Markdown
Contributor Author

@bkietz please, have a look to this PR

Comment thread cpp/src/arrow/compute/kernels/scalar_cast_test.cc Outdated
@lidavidm

lidavidm commented Aug 5, 2021

Copy link
Copy Markdown
Member

Looks like there are some lint failures - you can fix them locally as described at https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci or by commenting @github-actions autotune in this PR.

@lidavidm lidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this. I left a comment about the FIXME.

Comment thread cpp/src/arrow/compute/kernels/scalar_cast_test.cc Outdated
Comment thread cpp/src/arrow/compute/kernels/scalar_cast_test.cc

@lidavidm lidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks! Will merge on green.

@lidavidm

lidavidm commented Aug 5, 2021

Copy link
Copy Markdown
Member

(Well, if you don't plan on adding more - feel free to mark ready for review)

@rommelDB rommelDB marked this pull request as ready for review August 5, 2021 16:12
@rommelDB

rommelDB commented Aug 5, 2021

Copy link
Copy Markdown
Contributor Author

LGTM, thanks! Will merge on green.

Thank you so much David! I will be aware of the CI output.

@lidavidm

lidavidm commented Aug 5, 2021

Copy link
Copy Markdown
Member

I think the failures here are flukes (GHA seems to be having a bit of trouble), let's see when the Travis build finishes.

@lidavidm lidavidm closed this in a669ac5 Aug 5, 2021
@nealrichardson

Copy link
Copy Markdown
Member

Thanks for doing this! 🙏 In the future, it can be a good idea to grep the source for the Jira issue number to see if there are any TODOs or skipped tests related to the issue. There was one in the R test suite from when the issue was originally reported; I've removed it in #10891.

@rommelDB

rommelDB commented Aug 6, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for doing this! pray In the future, it can be a good idea to grep the source for the Jira issue number to see if there are any TODOs or skipped tests related to the issue. There was one in the R test suite from when the issue was originally reported; I've removed it in #10891.

Yes, I totally agree with you, thanks for the reminder.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants