Skip to content

Conversation

@seddonm1
Copy link
Contributor

This PR is a child of #9243

It does a few things that are hard to separate:

@alamb @jorgecarleitao
please review but merging will be dependent on #9507

@github-actions
Copy link

@seddonm1 seddonm1 changed the title ARROW-11738: Fix Concat and Trim Functions ARROW-11738: [Rust][DataFusion] Fix Concat and Trim Functions Feb 23, 2021
@alamb
Copy link
Contributor

alamb commented Feb 23, 2021

Thanks @seddonm1 -- I will plan to review this over the next few days

@seddonm1
Copy link
Contributor Author

@Dandandan I can add substr to this PR if that would help you with TCPH-q22?

@seddonm1
Copy link
Contributor Author

@Dandandan I have added substr to this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Alphabetically, this should be after concat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, my VSCode sorting treats _ before ". fixed 👍

Copy link
Contributor

@Dandandan Dandandan left a comment

Choose a reason for hiding this comment

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

LGTM!
I think from time to time we will can check on the status of the tpch queries. Feel free to split the PRs in the way you think makes sense @seddonm1 👍

@seddonm1
Copy link
Contributor Author

LGTM!
I think from time to time we will can check on the status of the tpch queries. Feel free to split the PRs in the way you think makes sense @seddonm1 👍

@Dandandan basically the string functions PR is so big even splitting like this makes a big difference and allows applying some of the foundational components (see the testing) required for the rest. So if this helps unblock you then thats a good thing :D

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.

Epic work @seddonm1 -- thank you again. I skimmed this and spot checked a few functions, but it all looks as good as #9243 is. I think this one is good to merge.

@alamb alamb added the needs-rebase A PR that needs to be rebased by the author label Feb 23, 2021
@alamb
Copy link
Contributor

alamb commented Feb 23, 2021

This PR sadly does need a rebase

@seddonm1
Copy link
Contributor Author

thanks @alamb rebased 👍

@alamb
Copy link
Contributor

alamb commented Feb 24, 2021

Integration test failure looks like https://issues.apache.org/jira/browse/ARROW-11717, so ignoring

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

Labels

Component: Rust - DataFusion Component: Rust needs-rebase A PR that needs to be rebased by the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants