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

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::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 Physical Expressions 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
23 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zy-kkk commented Jan 14, 2024 •

edited

Loading

Dandandan Jan 15, 2024

zy-kkk Jan 16, 2024

alamb Jan 18, 2024

Dandandan Jan 15, 2024

zy-kkk Jan 16, 2024

Omega359 Jan 17, 2024

alamb left a comment

alamb Jan 18, 2024

alamb Jan 18, 2024

zy-kkk commented Jan 22, 2024 •

edited

Loading

alamb commented Jan 22, 2024

alamb left a comment

alamb Jan 22, 2024

zy-kkk commented Jan 23, 2024

zy-kkk commented Jan 23, 2024

alamb commented Jan 23, 2024

alamb commented Jan 23, 2024

alamb left a comment

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

		BuiltinScalarFunction::InitCap => Volatility::Immutable,
		BuiltinScalarFunction::InStr => Volatility::Immutable,

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

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

Conversation

zy-kkk commented Jan 14, 2024 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zy-kkk commented Jan 22, 2024 • edited Loading

alamb commented Jan 22, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zy-kkk commented Jan 23, 2024

zy-kkk commented Jan 23, 2024

alamb commented Jan 23, 2024

alamb commented Jan 23, 2024

alamb left a comment

Choose a reason for hiding this comment

zy-kkk commented Jan 14, 2024 •

edited

Loading

zy-kkk commented Jan 22, 2024 •

edited

Loading