-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
chore: Migrate Regex function to invoke_with_args #14728
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.
Thanks @irenjj. Overall looks good to me, but I think there are some outdated annotations.
&[ColumnarValue::Scalar(v_sv), ColumnarValue::Scalar(regex_sv)], | ||
1, | ||
); | ||
#[allow(deprecated)] |
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 it is out of date. It's for inovke_batch
but we have migrated to inovek_with_args
. We can remove it.
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.
There are other similar parts. We can remove all of them.
#[allow(deprecated)] | ||
let re = RegexpCountFunc::new().invoke_with_args(ScalarFunctionArgs { | ||
args: vec![ColumnarValue::Scalar(v_sv), ColumnarValue::Scalar(regex_sv)], | ||
number_rows: 2, |
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 just noticed that it's different from the previous number 🤔. However, I think regexp_count
never uses the number of rows. It's fine.
Thanks @goldmedal , have modified according to your suggestion, PTAL. :) |
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.
Thanks @irenjj 👍 It looks good to me.
Which issue does this PR close?
invoke_with_args
#14707Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?