Skip to content

Comments

Implement native support StringView for CONTAINS function#12168

Merged
alamb merged 8 commits intoapache:mainfrom
tlm365:contains-string-view
Sep 10, 2024
Merged

Implement native support StringView for CONTAINS function#12168
alamb merged 8 commits intoapache:mainfrom
tlm365:contains-string-view

Conversation

@tlm365
Copy link
Contributor

@tlm365 tlm365 commented Aug 26, 2024

Which issue does this PR close?

Closes #11838.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Aug 26, 2024
@tlm365 tlm365 marked this pull request as draft August 26, 2024 10:13
@tlm365 tlm365 force-pushed the contains-string-view branch from 8508473 to 88aab46 Compare August 26, 2024 15:08
@tlm365 tlm365 force-pushed the contains-string-view branch from a71d222 to 45dd141 Compare August 26, 2024 17:47
@tlm365 tlm365 marked this pull request as ready for review August 26, 2024 18:32
@tlm365 tlm365 mentioned this pull request Aug 27, 2024
Closed
@Omega359
Copy link
Contributor

Omega359 commented Sep 2, 2024

I am wondering since the contains udf relies on regex (which isn't documented at all btw) whether it should be a wrapper for regexp_like and moved to the regex module? The order of the arguments is reversed and there is no flags for contains but functionally I think the two are equivalent.

@Omega359 Omega359 mentioned this pull request Sep 2, 2024
Closed
@tlm365
Copy link
Contributor Author

tlm365 commented Sep 4, 2024

I am wondering since the contains udf relies on regex (which isn't documented at all btw) whether it should be a wrapper for regexp_like and moved to the regex module? The order of the arguments is reversed and there is no flags for contains but functionally I think the two are equivalent.

yeah, I also mentioned the similar of contains and regex_like that you mentioned above in #12180. But somehow I chose to implement it in string module. Let me try to implement it in regex module as your point - I also think it makes more sense.

@tlm365 tlm365 marked this pull request as draft September 6, 2024 02:39
@tlm365 tlm365 force-pushed the contains-string-view branch 4 times, most recently from ce80864 to 22f383b Compare September 6, 2024 09:59
@tlm365 tlm365 marked this pull request as ready for review September 6, 2024 10:42
alamb
alamb reviewed Sep 6, 2024
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.

Thank you @tlm365 -- sorry for the delay in reviewing. Overall this PR looks very cleanly implemented and is easy to read 👌

I had a few questions, but it looks pretty close to me

Thanks again

datafusion/functions/src/regex/regexp_common.rs
/// See the documentation [here](https://docs.rs/regex/1.5.4/regex/#grouping-and-flags)
/// for more information.
///
/// It is inspired / copied from `regexp_is_match_utf8` [arrow-rs].
Copy link
Contributor

@alamb alamb Sep 6, 2024

Choose a reason for hiding this comment

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

I wonder why we need to haev our own copy here? Is it because the arrow kernel doesn't support StringView yet?

Copy link
Contributor Author

@tlm365 tlm365 Sep 7, 2024

Choose a reason for hiding this comment

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

Is it because the arrow kernel doesn't support StringView yet?

Yes, you're right. I intend to update this one in upstream [arrow-rs]. After that, we can eliminate it from this location.

datafusion/functions/Cargo.toml Outdated
math_expressions = []
# enable regular expressions
regex_expressions = ["regex"]
regex_expressions = ["regex", "string_expressions"]
Copy link
Contributor

@alamb alamb Sep 6, 2024

Choose a reason for hiding this comment

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

I don't understand the need for this change (which seems to basically make regex_expressions and string_expressions the same feature flag)

I see you added datafusion/functions/src/regex/regexp_common.rs, but that seems only to be used by string functions. Perhaps it would be beter placed somewhere in functions/src/ 🤔

Copy link
Contributor Author

@tlm365 tlm365 Sep 7, 2024

Choose a reason for hiding this comment

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

@alamb thanks for reviewing 🙏

Perhaps it would be beter placed somewhere in functions/src/

oh, I agree, will update it soon

@tlm365 tlm365 force-pushed the contains-string-view branch 2 times, most recently from f1bff73 to 6267b8d Compare September 7, 2024 00:41
@tlm365 tlm365 force-pushed the contains-string-view branch from 6267b8d to 4344bc8 Compare September 8, 2024 05:24
@alamb alamb mentioned this pull request Sep 9, 2024
Closed
alamb
alamb reviewed Sep 9, 2024
View reviewed changes
alamb
alamb approved these changes Sep 9, 2024
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.

Thank you @tlm365 -- looks great to me

@alamb alamb merged commit c71a9d7 into apache:main Sep 10, 2024
@alamb
Copy link
Contributor

alamb commented Sep 10, 2024

Thanks again @tlm365

@tlm365 tlm365 deleted the contains-string-view branch November 10, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

Assignees

No one assigned

Labels

documentation Improvements or additions to documentation functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update CONTAINS scalar function to support Utf8View

3 participants

@tlm365 @Omega359 @alamb