-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat:implement calcite style 'levenshtein' string function #8168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution @Syleechan . I think we can avoid the new dependency but otherwise this looks great
datafusion/physical-expr/Cargo.toml
Outdated
@@ -67,6 +67,7 @@ regex = { version = "1.8", optional = true } | |||
sha2 = { version = "^0.10.1", optional = true } | |||
unicode-segmentation = { version = "^1.7.1", optional = true } | |||
uuid = { version = "^1.2", features = ["v4"] } | |||
edit-distance = "2.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can avoid this new dependency by either reusing our existing implementation (see below)
# Conflicts: # datafusion/proto/proto/datafusion.proto # datafusion/proto/src/generated/pbjson.rs # datafusion/proto/src/generated/prost.rs # datafusion/proto/src/logical_plan/from_proto.rs
@alamb thanks, I have changed to use exsiting implementation. But the code checks maybe something wrong, I have used the checking command to run local all have passed. |
I believe the failures are due to some logical conflicts since fixed in #8187 Updating to the latest main should work I think |
# Conflicts: # datafusion/proto/proto/datafusion.proto # datafusion/proto/src/generated/pbjson.rs # datafusion/proto/src/generated/prost.rs
@alamb thanks, it works. And wait for your code review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me -- thank you @Syleechan
Which issue does this PR close?
Closes #.
Rationale for this change
https://calcite.apache.org/docs/reference.html#:~:text=LEVENSHTEIN(string1%2C%20string2,string1%20and%20string2
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?