Skip to content

Comments

ARROW-11738: [Rust][DataFusion] Fix Concat and Trim Functions#9551

Closed
seddonm1 wants to merge 1 commit intoapache:masterfrom
seddonm1:concat
Closed

ARROW-11738: [Rust][DataFusion] Fix Concat and Trim Functions#9551
seddonm1 wants to merge 1 commit intoapache:masterfrom
seddonm1:concat

Conversation

@seddonm1
Copy link
Contributor

@seddonm1 seddonm1 commented Feb 22, 2021

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

github-actions bot commented Feb 22, 2021

https://issues.apache.org/jira/browse/ARROW-11738

@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

seddonm1 commented Feb 23, 2021

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

@seddonm1
Copy link
Contributor Author

seddonm1 commented Feb 23, 2021

@Dandandan I have added substr to this PR

Dandandan
Dandandan reviewed Feb 23, 2021
View reviewed changes
rust/datafusion/src/physical_plan/functions.rs Outdated
Copy link
Contributor

@Dandandan Dandandan Feb 23, 2021

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

@seddonm1 seddonm1 Feb 23, 2021

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 👍

Dandandan
Dandandan approved these changes Feb 23, 2021
View reviewed changes
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

seddonm1 commented Feb 23, 2021

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

alamb
alamb approved these changes Feb 23, 2021
View reviewed changes
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

seddonm1 commented Feb 23, 2021

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

@alamb alamb closed this in 4de992c Feb 24, 2021
@asfimport asfimport mentioned this pull request Feb 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Dandandan Dandandan Dandandan approved these changes

@alamb alamb alamb approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@seddonm1 @alamb @Dandandan