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

Support string concat || for StringViewArray #12063

Merged
merged 10 commits into from
Aug 22, 2024

Conversation

dharanad
Copy link
Contributor

Which issue does this PR close?

Closes #11766

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@dharanad
Copy link
Contributor Author

@alamb I have come up with a naive implementation, and i want to benchmark with current baseline.
Can you guide me on benchmarking strategy and execution?

@github-actions github-actions bot added physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Aug 19, 2024
@alamb
Copy link
Contributor

alamb commented Aug 19, 2024

Thanks @dharanad -- this looks super exciting

Can you guide me on benchmarking strategy and execution?

In terms of benchmarking I would suggest make (in a separate PR) a cargo bench style benchmark, following the model of #12044. A separate PR makes it easier to run the benchmark before/after your changes

@dharanad dharanad marked this pull request as ready for review August 20, 2024 17:01
@dharanad
Copy link
Contributor Author

I'm not entirely confident about this approach, as the only method I can think of involves creating a new buffer to hold the concatenated string. Any feedback or guidance on a more efficient solution would be greatly appreciated.

@dharanad dharanad changed the title [WIP] Support string concat || for StringViewArray Support string concat || for StringViewArray Aug 20, 2024
@dharanad
Copy link
Contributor Author

Weird logictest are passing locally also the error cause is wrong.

@dharanad
Copy link
Contributor Author

@alamb / @XiangpengHao Can you please help me with a review

@alamb
Copy link
Contributor

alamb commented Aug 21, 2024

I will review this later today

@alamb
Copy link
Contributor

alamb commented Aug 21, 2024

Weird logictest are passing locally also the error cause is wrong.

Likewise I found it strange the tests were passing locally for me.

@dharanad, I took the liberty of merging up from main to get this branch to pass and wrote a few more tests. These tests actually uncovered a bug (value || NULL --> NULL but the previous implementation would return value)

I also made a small performance improvement by avoiding allocating a new String for each row

I also found another bug which I will file separately

Update: filed #12101

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 @dharanad 🙏

@alamb alamb changed the title Support string concat || for StringViewArray Support string concat || for StringViewArray Aug 21, 2024
@alamb alamb merged commit 87c1c17 into apache:main Aug 22, 2024
25 checks passed
@alamb
Copy link
Contributor

alamb commented Aug 22, 2024

Thanks again @dharanad

@dharanad
Copy link
Contributor Author

Thank You @alamb Really appreciate your help

@alamb
Copy link
Contributor

alamb commented Aug 24, 2024

Thank You @alamb Really appreciate your help

Likewise!

@dharanad dharanad deleted the feat/11766-stringview-concat branch August 28, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support string concat || for StringViewArray
2 participants