feat:implement sql style 'ends_with' and 'instr' string function by zy-kkk · Pull Request #8862 · apache/datafusion

zy-kkk · 2024-01-14T13:01:28Z

Which issue does this PR close?

Closes #.

Rationale for this change

instr: https://dev.mysql.com/doc/refman/8.0/en/string-functions.html#function_instr
ends_with: Refer to the existing starts_with function

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Dandandan · 2024-01-15T15:40:45Z

datafusion/physical-expr/src/string_expressions.rs

+/// Returns true if string ends with suffix.
+/// ends_with('alphabet', 'abet') = 't'
+pub fn ends_with<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+    let string_array = as_generic_string_array::<T>(&args[0])?;


We should be able to use https://docs.rs/arrow/latest/arrow/compute/kernels/comparison/fn.ends_with.html

Do you mean that I can call this library function directly in the code or do I not need to implement this function?

Do you mean that I can call this library function directly in the code or do I not need to implement this function?

Yes, I think the body of thus function could be reduced to something like

Suggested change

let string_array = as_generic_string_array::<T>(&args[0])?;

compute::comparison::ends_with(&args[0])

Dandandan · 2024-01-15T15:45:46Z

datafusion/expr/src/built_in_function.rs

            BuiltinScalarFunction::DateBin => Volatility::Immutable,
+            BuiltinScalarFunction::EndsWith => Volatility::Immutable,
            BuiltinScalarFunction::InitCap => Volatility::Immutable,
+            BuiltinScalarFunction::InStr => Volatility::Immutable,


PostgreSQL has POSITION (see https://www.postgresql.org/docs/9.1/functions-string.html)

Does the function name we implement need to follow postgresql? If so, I will change the name of this function

Spark has both position and instr and they are not quite the same signature:
https://spark.apache.org/docs/latest/api/sql/#position
https://spark.apache.org/docs/latest/api/sql/#instr

alamb

Thank you very much for this PR @zy-kkk -- it is very well tested and commented 🦾 .

It looks quite close to me. The only thing I think it needs prior to merging is to address the potential use of an arrow kernel rather than reimplementation for ends_with but otherwise it is ready to go 🚀

Thanks again

alamb · 2024-01-18T19:20:43Z

datafusion/physical-expr/src/string_expressions.rs

+/// Returns true if string ends with suffix.
+/// ends_with('alphabet', 'abet') = 't'
+pub fn ends_with<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+    let string_array = as_generic_string_array::<T>(&args[0])?;


Do you mean that I can call this library function directly in the code or do I not need to implement this function?

Yes, I think the body of thus function could be reduced to something like

Suggested change

let string_array = as_generic_string_array::<T>(&args[0])?;

compute::comparison::ends_with(&args[0])

datafusion/physical-expr/src/string_expressions.rs

alamb · 2024-01-18T19:26:38Z

datafusion/physical-expr/src/functions.rs

+            Boolean,
+            BooleanArray
+        );
+        test_function!(


💯 for testing with NULL

datafusion/physical-expr/src/functions.rs

…tations and extend tests for instr function

zy-kkk · 2024-01-22T13:40:13Z

Hello @alamb, first of all thank you for your review of my PR,
I have used the arrow comparison function for the ends_with function implementation, and by the way changed starts_with to this.
For the instr function, I added more tests, but for this function, its implementation in MySQL is: instr('Helloworld', 'world') = 6, and for postgresql, it is, position('world' in 'Helloworld') = 6, which one do you want me to use, please tell me, currently I still retain the MySQL design
Thanks again for your review

alamb · 2024-01-22T21:22:41Z

Hello @alamb, first of all thank you for your review of my PR, I have used the arrow comparison function for the ends_with function implementation, and by the way changed starts_with to this. For the instr function, I added more tests, but for this function, its implementation in MySQL is: instr('Helloworld', 'world') = 6, and for postgresql, it is, position('world' in 'Helloworld') = 6, which one do you want me to use, please tell me, currently I still retain the MySQL design Thanks again for your review

Thank you @zy-kkk -- I think we try to follow the postgres style, but making the function packages as compatible as possible is a good thing.

In this case, perhaps you can add an alias for instr so it can also be called with position: https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.BuiltinScalarFunction.html#method.aliases

alamb

Thank you very much @zy-kkk -- I reviewed this PR and I think it looks quite good.

It would be nice to add a position alias, but also think we can do that as a follow on PR as well

alamb · 2024-01-22T21:19:58Z

datafusion/physical-expr/src/string_expressions.rs

-            _ => None,
-        })
-        .collect::<BooleanArray>();
+    let result = arrow::compute::kernels::comparison::starts_with(left, right)?;


zy-kkk · 2024-01-23T13:12:49Z

Thank you very much @zy-kkk -- I reviewed this PR and I think it looks quite good.

It would be nice to add a position alias, but also think we can do that as a follow on PR as well

Thank you, @alamb, I will continue to modify this PR so that position('world' in 'Helloworld') = 6 can be run to be compatible with postgresql

zy-kkk · 2024-01-23T15:15:09Z

Thank you very much @zy-kkk -- I reviewed this PR and I think it looks quite good.
It would be nice to add a position alias, but also think we can do that as a follow on PR as well

Thank you, @alamb, I will continue to modify this PR so that position('world' in 'Helloworld') = 6 can be run to be compatible with postgresql

Hi @alamb, I tested changing instr to position. If I want to support the syntax position('world' in 'Helloworld') = 6, I need to do it at https://github.com/sqlparser-rs/sqlparser-rs Add a syntax parsing in , which may be like adding the overlay function in apache/datafusion-sqlparser-rs#594. What do you suggest?

alamb · 2024-01-23T15:59:48Z

If I want to support the syntax position('world' in 'Helloworld') = 6, I need to do it at https://github.com/sqlparser-rs/sqlparser-rs Add a syntax parsing in , which may be like adding the overlay function in apache/datafusion-sqlparser-rs#594. What do you suggest?

Hi @zy-kkk -- I didn't realize there was special syntax. Yes in order to support that special syntax we would need a change in sqlparser.

What I suggest in this case is that we file a follow on ticket to support posgresql style and merge this PR. I will file the ticket shortly

alamb · 2024-01-23T16:04:30Z

I filed #8969 to track adding the position function

alamb

Thanks again @zy-kkk -- this is a really nice addition

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jan 14, 2024

zy-kkk force-pushed the string_fun branch from 4d3168f to f584663 Compare January 14, 2024 13:31

alamb mentioned this pull request Jan 15, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 15, 2024 #8864

Closed

9 tasks

Dandandan reviewed Jan 15, 2024

View reviewed changes

alamb reviewed Jan 18, 2024

View reviewed changes

zy-kkk added 2 commits January 22, 2024 20:58

feat:implement sql style 'ends_with' and 'instr' string function

c53a5e9

Use Arrow comparison functions for ends_with and starts_with implemen…

6e6cb31

…tations and extend tests for instr function

zy-kkk force-pushed the string_fun branch from f584663 to 6e6cb31 Compare January 22, 2024 13:35

zy-kkk requested a review from alamb January 22, 2024 13:40

alamb mentioned this pull request Jan 22, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 22, 2024 #8933

Closed

9 tasks

alamb approved these changes Jan 22, 2024

View reviewed changes

alamb mentioned this pull request Jan 23, 2024

Implement the POSITION function #8969

Closed

alamb approved these changes Jan 23, 2024

View reviewed changes

alamb merged commit 084fdfb into apache:main Jan 23, 2024

zy-kkk deleted the string_fun branch January 23, 2024 16:33

This was referenced Jan 23, 2024

Fix clippy failures: error: use of deprecated function `functions::make_scalar_function #8972

Merged

Clippy failures on main: error: use of deprecated function functions::make_scalar_function` #8973

Closed

	let string_array = as_generic_string_array::<T>(&args[0])?;
	compute::comparison::ends_with(&args[0])

Conversation

zy-kkk commented Jan 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zy-kkk commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Jan 22, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zy-kkk commented Jan 23, 2024

Uh oh!

zy-kkk commented Jan 23, 2024

Uh oh!

alamb commented Jan 23, 2024

Uh oh!

alamb commented Jan 23, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zy-kkk commented Jan 14, 2024 •

edited

Loading

zy-kkk commented Jan 22, 2024 •

edited

Loading