You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/02/05 17:17:07 UTC
[GitHub] [arrow] sweb opened a new pull request #9428: ARROW-10354: [Rust] regexp_extract function to select regex groups from strings
sweb opened a new pull request #9428:
URL: https://github.com/apache/arrow/pull/9428
Still WIP: Adds a regexp_extract compute kernel to select a substring based on a regular expression.
Open issues:
* [ ] Expose functionality and add to datafusion
* [ ] Additional tests
* [ ] Better doc string
Some things I did that I may be doing wrong:
* I exposed `GenericStringBuilder`
* I build the resulting Array using a builder - this looks quite different from e.g. the substring kernel. Should I change it accordingly, e.g. because of performance considerations?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r603656164
##########
File path: rust/datafusion/tests/sql.rs
##########
@@ -2607,3 +2607,24 @@ async fn invalid_qualified_table_references() -> Result<()> {
}
Ok(())
}
+
+#[tokio::test]
+#[cfg(feature = "regex_expressions")]
+async fn query_regexp_match() -> Result<()> {
Review comment:
It think we can remove this and just add these cases to the `test_regex_expressions` function above:
```rust
test_expression!("regexp_match('foobarbequebaz', '')", "[]");
test_expression!("regexp_match('foobarbequebaz', '(bar)(beque)')", "[bar, beque]");
test_expression!("regexp_match('foobarbequebaz', '(ba3r)(bequ34e)')", "NULL");
test_expression!("regexp_match('aaa-0', '.*-(\\d)')", "[0]");
test_expression!("regexp_match('bb-1', '.*-(\\d)')", "[1]");
test_expression!("regexp_match('aa', '.*-(\\d)')", "NULL");
test_expression!("regexp_match(NULL, '.*-(\\d)')", "NULL");
test_expression!("regexp_match('aaa-0', NULL)", "NULL");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-795094945
> @sweb I know you have spent a lot of time on this so I know its painful but I do think that we should built to the Postgres spec which will allow regex to be passed in via a columnar value. How we implement that behind the scene allows optimisation but the spec allows each row to be individually processed with a different regex.
>
> To clarify: I am not sure if anyone _actually_ uses SQL like this but it is a consistent implementation pattern in Postgres where a Scalar and Columnar value is treated equally - which I think is an elegant design.
@seddonm1 all good, thanks for your reply. I just wanted to make sure that the kernel is useful. I will adjust it accordingly and maybe we can refactor some stuff later on, since I will probably copy several parts of your implementation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb closed pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
alamb closed pull request #9428:
URL: https://github.com/apache/arrow/pull/9428
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-796301063
@sweb this looks like it is getting there 👍
The flags are super messy in the SQL Posix fmt vs the Rust regex so you have that to look forward to. We do need to try to reuse some code but unfortunately that means either lifting the current regexp_expressions logic into a kernel or the moving your logic to the regexp file.
I think if you can get to the point of making it work and lots of test then we can always refactor for performance later (as we better understand the problems).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r602608787
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ ArrayRef, GenericStringArray, GenericStringBuilder, ListBuilder,
+ StringOffsetSizeTrait,
+};
+use crate::error::{ArrowError, Result};
+use std::collections::HashMap;
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+/// Extract all groups matched by a regular expression for a given String array.
+pub fn regexp_match<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ regex_array: &GenericStringArray<OffsetSize>,
+ flags_array: Option<&GenericStringArray<OffsetSize>>,
+) -> Result<ArrayRef> {
+ let mut patterns: HashMap<String, Regex> = HashMap::new();
+ let builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+ let mut list_builder = ListBuilder::new(builder);
+
+ let complete_pattern = match flags_array {
+ Some(flags) => Box::new(regex_array.iter().zip(flags.iter()).map(
+ |(pattern, flags)| {
+ pattern.map(|pattern| match flags {
+ Some(value) => format!("(?{}){}", value, pattern),
+ None => pattern.to_string(),
+ })
+ },
+ )) as Box<dyn Iterator<Item = Option<String>>>,
+ None => Box::new(
+ regex_array
+ .iter()
+ .map(|pattern| pattern.map(|pattern| pattern.to_string())),
+ ),
+ };
+ array
+ .iter()
+ .zip(complete_pattern)
+ .map(|(value, pattern)| {
+ match (value, pattern) {
+ (Some(value), Some(pattern)) => {
+ let existing_pattern = patterns.get(&pattern);
+ let re = match existing_pattern {
+ Some(re) => re.clone(),
+ None => {
+ let re = Regex::new(pattern.as_str()).map_err(|e| {
+ ArrowError::ComputeError(format!(
+ "Regular expression did not compile: {:?}",
+ e
+ ))
+ })?;
+ patterns.insert(pattern, re.clone());
+ re
+ }
+ };
+ match re.captures(value) {
+ Some(caps) => {
+ for m in caps.iter().skip(1) {
+ if let Some(v) = m {
+ list_builder.values().append_value(v.as_str())?;
+ }
+ }
+ list_builder.append(true)?
+ }
+ None => {
+ list_builder.values().append_value("")?;
+ list_builder.append(true)?
+ }
+ }
+ }
+ _ => list_builder.append(false)?,
+ }
+ Ok(())
+ })
+ .collect::<Result<Vec<()>>>()?;
+ Ok(Arc::new(list_builder.finish()))
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::array::{ListArray, StringArray};
+
+ #[test]
+ fn match_single_group() -> Result<()> {
Review comment:
Could you also add a test case for and (`regexp_match('foobarbequebaz', '(bar)(bequ1e)')` above):
```sql
SELECT regexp_match('foobarbequebaz', ''); = {""}
```
Some of these behaviors from Postgres don't really make sense to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-809756978
@sweb This is great and does work with all the cases I have run tests for :+1:. You can see my slight suggested change above and once thats done @alamb this is ready for merge.
Sorry for being pedantic but I think this is really important functionality.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-791739578
@sweb I can help on Monday. I'm planning to raise the PR for those other regexp functions then can help work through this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r588468947
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ Array, ArrayRef, GenericStringArray, GenericStringBuilder, LargeStringArray,
+ StringArray, StringOffsetSizeTrait,
+};
+use crate::datatypes::DataType;
+use crate::error::{ArrowError, Result};
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+fn generic_regexp_extract<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ re: &Regex,
+ idx: usize,
+) -> Result<ArrayRef> {
+ let mut builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+
+ for maybe_value in array.iter() {
Review comment:
I did not find a way to do this for `ListArray` - I think I would have to implement the `FromIterator` trait for `GenericListArray`, correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-799561986
@seddonm1 I have added flags and got the all-literals case to work. This is heavily copied from your implementation. Would you take another look?
I can also try to take a look at the `FromIterator` support for `ListArray` to avoid the usage of the builder but I am not sure how much of a change that would be.
Thank you for your patience with me!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r605182786
##########
File path: rust/datafusion/tests/sql.rs
##########
@@ -2607,3 +2607,24 @@ async fn invalid_qualified_table_references() -> Result<()> {
}
Ok(())
}
+
+#[tokio::test]
+#[cfg(feature = "regex_expressions")]
+async fn query_regexp_match() -> Result<()> {
Review comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r577296571
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ Array, ArrayRef, GenericStringArray, GenericStringBuilder, LargeStringArray,
+ StringArray, StringOffsetSizeTrait,
+};
+use crate::datatypes::DataType;
+use crate::error::{ArrowError, Result};
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+fn generic_regexp_extract<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ re: &Regex,
+ idx: usize,
+) -> Result<ArrayRef> {
+ let mut builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+
+ for maybe_value in array.iter() {
Review comment:
It is more efficient (and probably idiomatic) to use the `collect::<GenericStringArray<OffsetSize>>()` here.
I.e.
```rust
array
.iter()
.map(<logic here>)
.collect::<GenericStringArray<OffsetSize>>()
```
Since this is an unary operation on a utf8 array, I would try to write a generic for it (like we do for primitives in `arity.rs`) and use it here. We may even be able to write it using the `trusted_len`, which is the faster option available atm.
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ Array, ArrayRef, GenericStringArray, GenericStringBuilder, LargeStringArray,
+ StringArray, StringOffsetSizeTrait,
+};
+use crate::datatypes::DataType;
+use crate::error::{ArrowError, Result};
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+fn generic_regexp_extract<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ re: &Regex,
+ idx: usize,
+) -> Result<ArrayRef> {
+ let mut builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+
+ for maybe_value in array.iter() {
+ match maybe_value {
+ Some(value) => match re.captures(value) {
+ Some(caps) => {
+ let m = caps.get(idx).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Regexp has no group with index {}",
+ idx
+ ))
+ })?;
+ builder.append_value(m.as_str())?
+ }
+ None => builder.append_null()?,
+ },
+ None => builder.append_null()?,
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
+/// Extracts a specific group matched by a regular expression for a given String array.
+/// Group index 0 returns the whole match, index 1 returns the first group and so on. Please
+/// refer to regex crate for details on pattern specifics.
+pub fn regexp_extract(array: &Array, pattern: &str, idx: usize) -> Result<ArrayRef> {
Review comment:
I wonder if the signature shouldn't be `idx: &[usize]` and the result `Vec<ArrayRef>`. It would allow for optimizations where the user wants more than one group for the same regex (as regex is usually slow). Could be left out for now, just a though.
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ Array, ArrayRef, GenericStringArray, GenericStringBuilder, LargeStringArray,
+ StringArray, StringOffsetSizeTrait,
+};
+use crate::datatypes::DataType;
+use crate::error::{ArrowError, Result};
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+fn generic_regexp_extract<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ re: &Regex,
+ idx: usize,
+) -> Result<ArrayRef> {
+ let mut builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+
+ for maybe_value in array.iter() {
+ match maybe_value {
+ Some(value) => match re.captures(value) {
+ Some(caps) => {
+ let m = caps.get(idx).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Regexp has no group with index {}",
+ idx
+ ))
+ })?;
+ builder.append_value(m.as_str())?
+ }
+ None => builder.append_null()?,
+ },
+ None => builder.append_null()?,
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
+/// Extracts a specific group matched by a regular expression for a given String array.
+/// Group index 0 returns the whole match, index 1 returns the first group and so on. Please
+/// refer to regex crate for details on pattern specifics.
+pub fn regexp_extract(array: &Array, pattern: &str, idx: usize) -> Result<ArrayRef> {
+ let re = Regex::new(pattern).map_err(|e| {
+ ArrowError::ComputeError(format!("Regular expression did not compile: {:?}", e))
+ })?;
+ match array.data_type() {
+ DataType::LargeUtf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<LargeStringArray>()
+ .expect("A large string is expected"),
+ &re,
+ idx,
+ ),
+ DataType::Utf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .expect("A string is expected"),
+ &re,
+ idx,
+ ),
+ _ => Err(ArrowError::ComputeError(format!(
+ "regexp_extract does not support type {:?}",
+ array.data_type()
+ ))),
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn extract_single_group() -> Result<()> {
+ let values = vec!["abc-005-def", "X-7-5", "X545"];
Review comment:
Can we make one of these entries `None`, so that we also test the null entry case?
##########
File path: rust/datafusion/src/physical_plan/string_expressions.rs
##########
@@ -70,6 +71,16 @@ pub fn concatenate(args: &[ArrayRef]) -> Result<StringArray> {
Ok(builder.finish())
}
+/// extract a specific group from a string column, using a regular expression
+pub fn regexp_extract(args: &[ArrayRef]) -> Result<ArrayRef> {
+ let pattern_expr = args[1].as_any().downcast_ref::<StringArray>().unwrap();
+ let pattern = pattern_expr.value(0);
+ let idx_expr = args[2].as_any().downcast_ref::<Int64Array>().unwrap();
+ let idx = idx_expr.value(0) as usize;
Review comment:
I am not sure this is correct: the function expects an array, but then only picks the first element of the array for the regex. Maybe this was used because ScalarFunctions did not support the `ScalarValue` variant?
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ Array, ArrayRef, GenericStringArray, GenericStringBuilder, LargeStringArray,
+ StringArray, StringOffsetSizeTrait,
+};
+use crate::datatypes::DataType;
+use crate::error::{ArrowError, Result};
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+fn generic_regexp_extract<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ re: &Regex,
+ idx: usize,
+) -> Result<ArrayRef> {
+ let mut builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+
+ for maybe_value in array.iter() {
+ match maybe_value {
+ Some(value) => match re.captures(value) {
+ Some(caps) => {
+ let m = caps.get(idx).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Regexp has no group with index {}",
+ idx
+ ))
+ })?;
+ builder.append_value(m.as_str())?
+ }
+ None => builder.append_null()?,
+ },
+ None => builder.append_null()?,
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
+/// Extracts a specific group matched by a regular expression for a given String array.
+/// Group index 0 returns the whole match, index 1 returns the first group and so on. Please
+/// refer to regex crate for details on pattern specifics.
+pub fn regexp_extract(array: &Array, pattern: &str, idx: usize) -> Result<ArrayRef> {
+ let re = Regex::new(pattern).map_err(|e| {
+ ArrowError::ComputeError(format!("Regular expression did not compile: {:?}", e))
+ })?;
+ match array.data_type() {
+ DataType::LargeUtf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<LargeStringArray>()
+ .expect("A large string is expected"),
+ &re,
+ idx,
+ ),
+ DataType::Utf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .expect("A string is expected"),
+ &re,
+ idx,
+ ),
+ _ => Err(ArrowError::ComputeError(format!(
+ "regexp_extract does not support type {:?}",
+ array.data_type()
+ ))),
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn extract_single_group() -> Result<()> {
+ let values = vec!["abc-005-def", "X-7-5", "X545"];
+ let array = StringArray::from(values);
+ let pattern = r".*-(\d*)-.*";
+ let actual = regexp_extract(&array, pattern, 1)?;
+ let expected = StringArray::from(vec![Some("005"), Some("7"), None]);
Review comment:
if we want to mimic spark (imo we should), the last entry should result in an empty string, not a `None`. This is because it would be otherwise impossible to differentiate between a "no match" and a "input is null".
##########
File path: rust/datafusion/src/physical_plan/functions.rs
##########
@@ -523,6 +544,7 @@ fn signature(fun: &BuiltinScalarFunction) -> Signature {
BuiltinScalarFunction::NullIf => {
Signature::Uniform(2, SUPPORTED_NULLIF_TYPES.to_vec())
}
+ BuiltinScalarFunction::RegexpExtract => Signature::Any(3),
Review comment:
I am not sure this is correct; isn't the signature `Variant([[Utf8, Utf8, UInt64], [LargeUtf8, Utf8, UInt64]])` or something like that?
Note that these signatures are very important because they are used for type validation during logical planning, as well as type coercion at physical planning. Whenever we write `Any`, the logical planning will accept any type. Worse, the type coercer will not perform any coercion.
In this case, because we downcast arg[2] to `Int64Array`, if the user passes a `Int32Array`, the execution panics.
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ Array, ArrayRef, GenericStringArray, GenericStringBuilder, LargeStringArray,
+ StringArray, StringOffsetSizeTrait,
+};
+use crate::datatypes::DataType;
+use crate::error::{ArrowError, Result};
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+fn generic_regexp_extract<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ re: &Regex,
+ idx: usize,
+) -> Result<ArrayRef> {
+ let mut builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+
+ for maybe_value in array.iter() {
+ match maybe_value {
+ Some(value) => match re.captures(value) {
+ Some(caps) => {
+ let m = caps.get(idx).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Regexp has no group with index {}",
+ idx
+ ))
+ })?;
+ builder.append_value(m.as_str())?
+ }
+ None => builder.append_null()?,
+ },
+ None => builder.append_null()?,
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
+/// Extracts a specific group matched by a regular expression for a given String array.
+/// Group index 0 returns the whole match, index 1 returns the first group and so on. Please
+/// refer to regex crate for details on pattern specifics.
+pub fn regexp_extract(array: &Array, pattern: &str, idx: usize) -> Result<ArrayRef> {
+ let re = Regex::new(pattern).map_err(|e| {
+ ArrowError::ComputeError(format!("Regular expression did not compile: {:?}", e))
+ })?;
+ match array.data_type() {
+ DataType::LargeUtf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<LargeStringArray>()
+ .expect("A large string is expected"),
+ &re,
+ idx,
+ ),
+ DataType::Utf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .expect("A string is expected"),
+ &re,
+ idx,
+ ),
+ _ => Err(ArrowError::ComputeError(format!(
+ "regexp_extract does not support type {:?}",
+ array.data_type()
+ ))),
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn extract_single_group() -> Result<()> {
+ let values = vec!["abc-005-def", "X-7-5", "X545"];
+ let array = StringArray::from(values);
+ let pattern = r".*-(\d*)-.*";
+ let actual = regexp_extract(&array, pattern, 1)?;
+ let expected = StringArray::from(vec![Some("005"), Some("7"), None]);
+ let result = actual.as_any().downcast_ref::<StringArray>().unwrap();
+ assert_eq!(&expected, result);
+ Ok(())
+ }
+
+ #[test]
+ fn no_matches() -> Result<()> {
+ let values = vec!["abc", "X::50::00", "X545"];
+ let array = StringArray::from(values);
+ let pattern = r".*-(\d*)-.*";
+ let actual = regexp_extract(&array, pattern, 1)?;
+ let expected = StringArray::from(vec![None, None, None]);
+ let result = actual.as_any().downcast_ref::<StringArray>().unwrap();
+ assert_eq!(&expected, result);
+ Ok(())
+ }
Review comment:
I would be fine not adding this test. IMO this is covered on the test above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r602892170
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ ArrayRef, GenericStringArray, GenericStringBuilder, ListBuilder,
+ StringOffsetSizeTrait,
+};
+use crate::error::{ArrowError, Result};
+use std::collections::HashMap;
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+/// Extract all groups matched by a regular expression for a given String array.
+pub fn regexp_match<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ regex_array: &GenericStringArray<OffsetSize>,
+ flags_array: Option<&GenericStringArray<OffsetSize>>,
+) -> Result<ArrayRef> {
+ let mut patterns: HashMap<String, Regex> = HashMap::new();
+ let builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+ let mut list_builder = ListBuilder::new(builder);
+
+ let complete_pattern = match flags_array {
+ Some(flags) => Box::new(regex_array.iter().zip(flags.iter()).map(
+ |(pattern, flags)| {
+ pattern.map(|pattern| match flags {
+ Some(value) => format!("(?{}){}", value, pattern),
+ None => pattern.to_string(),
+ })
+ },
+ )) as Box<dyn Iterator<Item = Option<String>>>,
+ None => Box::new(
+ regex_array
+ .iter()
+ .map(|pattern| pattern.map(|pattern| pattern.to_string())),
+ ),
+ };
+ array
+ .iter()
+ .zip(complete_pattern)
+ .map(|(value, pattern)| {
+ match (value, pattern) {
+ (Some(value), Some(pattern)) => {
+ let existing_pattern = patterns.get(&pattern);
+ let re = match existing_pattern {
+ Some(re) => re.clone(),
+ None => {
+ let re = Regex::new(pattern.as_str()).map_err(|e| {
+ ArrowError::ComputeError(format!(
+ "Regular expression did not compile: {:?}",
+ e
+ ))
+ })?;
+ patterns.insert(pattern, re.clone());
+ re
+ }
+ };
+ match re.captures(value) {
+ Some(caps) => {
+ for m in caps.iter().skip(1) {
+ if let Some(v) = m {
+ list_builder.values().append_value(v.as_str())?;
+ }
+ }
+ list_builder.append(true)?
+ }
+ None => {
+ list_builder.values().append_value("")?;
+ list_builder.append(true)?
+ }
+ }
+ }
+ _ => list_builder.append(false)?,
+ }
+ Ok(())
+ })
+ .collect::<Result<Vec<()>>>()?;
+ Ok(Arc::new(list_builder.finish()))
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::array::{ListArray, StringArray};
+
+ #[test]
+ fn match_single_group() -> Result<()> {
Review comment:
@seddonm1 First: I am very impressed that you know of this case.
My current implementation returns an empty List, without an item. Do you know whether Postgres actually returns a quoted empty string? I am asking because
```
SELECT regexp_match('foobarbequebaz', '(bar)(beque)'); => {bar,beque}
```
so I am not sure what to make of the quotes, since strings are not returned with quotes or is this just a special case when the string is empty?
Regardless, I added special case for the empty string pattern
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r588467582
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ Array, ArrayRef, GenericStringArray, GenericStringBuilder, LargeStringArray,
+ StringArray, StringOffsetSizeTrait,
+};
+use crate::datatypes::DataType;
+use crate::error::{ArrowError, Result};
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+fn generic_regexp_extract<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ re: &Regex,
+ idx: usize,
+) -> Result<ArrayRef> {
+ let mut builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+
+ for maybe_value in array.iter() {
+ match maybe_value {
+ Some(value) => match re.captures(value) {
+ Some(caps) => {
+ let m = caps.get(idx).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Regexp has no group with index {}",
+ idx
+ ))
+ })?;
+ builder.append_value(m.as_str())?
+ }
+ None => builder.append_null()?,
+ },
+ None => builder.append_null()?,
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
+/// Extracts a specific group matched by a regular expression for a given String array.
+/// Group index 0 returns the whole match, index 1 returns the first group and so on. Please
+/// refer to regex crate for details on pattern specifics.
+pub fn regexp_extract(array: &Array, pattern: &str, idx: usize) -> Result<ArrayRef> {
+ let re = Regex::new(pattern).map_err(|e| {
+ ArrowError::ComputeError(format!("Regular expression did not compile: {:?}", e))
+ })?;
+ match array.data_type() {
+ DataType::LargeUtf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<LargeStringArray>()
+ .expect("A large string is expected"),
+ &re,
+ idx,
+ ),
+ DataType::Utf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .expect("A string is expected"),
+ &re,
+ idx,
+ ),
+ _ => Err(ArrowError::ComputeError(format!(
+ "regexp_extract does not support type {:?}",
+ array.data_type()
+ ))),
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn extract_single_group() -> Result<()> {
+ let values = vec!["abc-005-def", "X-7-5", "X545"];
+ let array = StringArray::from(values);
+ let pattern = r".*-(\d*)-.*";
+ let actual = regexp_extract(&array, pattern, 1)?;
+ let expected = StringArray::from(vec![Some("005"), Some("7"), None]);
Review comment:
Done for `regexp_match`. This will now lead to a `ListArray([StringArray([""]])`, i.e. the group has a single entry with an empty string - even if there are multiple groups. I am not sure how Postgres behaves in this case... I will try to check
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r602892170
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ ArrayRef, GenericStringArray, GenericStringBuilder, ListBuilder,
+ StringOffsetSizeTrait,
+};
+use crate::error::{ArrowError, Result};
+use std::collections::HashMap;
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+/// Extract all groups matched by a regular expression for a given String array.
+pub fn regexp_match<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ regex_array: &GenericStringArray<OffsetSize>,
+ flags_array: Option<&GenericStringArray<OffsetSize>>,
+) -> Result<ArrayRef> {
+ let mut patterns: HashMap<String, Regex> = HashMap::new();
+ let builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+ let mut list_builder = ListBuilder::new(builder);
+
+ let complete_pattern = match flags_array {
+ Some(flags) => Box::new(regex_array.iter().zip(flags.iter()).map(
+ |(pattern, flags)| {
+ pattern.map(|pattern| match flags {
+ Some(value) => format!("(?{}){}", value, pattern),
+ None => pattern.to_string(),
+ })
+ },
+ )) as Box<dyn Iterator<Item = Option<String>>>,
+ None => Box::new(
+ regex_array
+ .iter()
+ .map(|pattern| pattern.map(|pattern| pattern.to_string())),
+ ),
+ };
+ array
+ .iter()
+ .zip(complete_pattern)
+ .map(|(value, pattern)| {
+ match (value, pattern) {
+ (Some(value), Some(pattern)) => {
+ let existing_pattern = patterns.get(&pattern);
+ let re = match existing_pattern {
+ Some(re) => re.clone(),
+ None => {
+ let re = Regex::new(pattern.as_str()).map_err(|e| {
+ ArrowError::ComputeError(format!(
+ "Regular expression did not compile: {:?}",
+ e
+ ))
+ })?;
+ patterns.insert(pattern, re.clone());
+ re
+ }
+ };
+ match re.captures(value) {
+ Some(caps) => {
+ for m in caps.iter().skip(1) {
+ if let Some(v) = m {
+ list_builder.values().append_value(v.as_str())?;
+ }
+ }
+ list_builder.append(true)?
+ }
+ None => {
+ list_builder.values().append_value("")?;
+ list_builder.append(true)?
+ }
+ }
+ }
+ _ => list_builder.append(false)?,
+ }
+ Ok(())
+ })
+ .collect::<Result<Vec<()>>>()?;
+ Ok(Arc::new(list_builder.finish()))
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::array::{ListArray, StringArray};
+
+ #[test]
+ fn match_single_group() -> Result<()> {
Review comment:
@seddonm1 First: I am very impressed that you know of this case.
My original implementation returned an empty List, without an item. Do you know whether Postgres actually returns a quoted empty string? I am asking because
```
SELECT regexp_match('foobarbequebaz', '(bar)(beque)'); => {bar,beque}
```
so I am not sure what to make of the quotes, since strings are not returned with quotes or is this just a special case when the string is empty?
Regardless, I added special case for the empty string pattern
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-812281703
@sweb Sorry I missed this in the review but raised https://github.com/apache/arrow/pull/9874 to fix the location of the test.
Great work!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r602875955
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ ArrayRef, GenericStringArray, GenericStringBuilder, ListBuilder,
+ StringOffsetSizeTrait,
+};
+use crate::error::{ArrowError, Result};
+use std::collections::HashMap;
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+/// Extract all groups matched by a regular expression for a given String array.
+pub fn regexp_match<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ regex_array: &GenericStringArray<OffsetSize>,
+ flags_array: Option<&GenericStringArray<OffsetSize>>,
+) -> Result<ArrayRef> {
+ let mut patterns: HashMap<String, Regex> = HashMap::new();
+ let builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+ let mut list_builder = ListBuilder::new(builder);
+
+ let complete_pattern = match flags_array {
+ Some(flags) => Box::new(regex_array.iter().zip(flags.iter()).map(
+ |(pattern, flags)| {
+ pattern.map(|pattern| match flags {
+ Some(value) => format!("(?{}){}", value, pattern),
+ None => pattern.to_string(),
+ })
+ },
+ )) as Box<dyn Iterator<Item = Option<String>>>,
+ None => Box::new(
+ regex_array
+ .iter()
+ .map(|pattern| pattern.map(|pattern| pattern.to_string())),
+ ),
+ };
+ array
+ .iter()
+ .zip(complete_pattern)
+ .map(|(value, pattern)| {
+ match (value, pattern) {
+ (Some(value), Some(pattern)) => {
+ let existing_pattern = patterns.get(&pattern);
+ let re = match existing_pattern {
+ Some(re) => re.clone(),
+ None => {
+ let re = Regex::new(pattern.as_str()).map_err(|e| {
+ ArrowError::ComputeError(format!(
+ "Regular expression did not compile: {:?}",
+ e
+ ))
+ })?;
+ patterns.insert(pattern, re.clone());
+ re
+ }
+ };
+ match re.captures(value) {
+ Some(caps) => {
+ for m in caps.iter().skip(1) {
+ if let Some(v) = m {
+ list_builder.values().append_value(v.as_str())?;
+ }
+ }
+ list_builder.append(true)?
+ }
+ None => {
+ list_builder.values().append_value("")?;
Review comment:
I changed the behavior - it returns NULL now. This was my half-thought-through attempt to address the review comment differentiating between matching on NULL and no match.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-805451302
@sweb yes, i will have a look in the next day or two
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-786918727
@sweb what is the status of this PR? Are you blocked? If you just haven't had time or inclination to make changes, that is fine (I totally understand) I just wanted try and clear the PR queue
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] codecov-io commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-808895199
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=h1) Report
> Merging [#9428](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=desc) (077a7dc) into [master](https://codecov.io/gh/apache/arrow/commit/894dd17c9602439c2b84c0b849fb0966606ceb1c?el=desc) (894dd17) will **increase** coverage by `0.02%`.
> The diff coverage is `92.71%`.
[](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9428 +/- ##
==========================================
+ Coverage 82.42% 82.44% +0.02%
==========================================
Files 252 253 +1
Lines 58977 59128 +151
==========================================
+ Hits 48609 48751 +142
- Misses 10368 10377 +9
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `83.39% <ø> (ø)` | |
| [rust/datafusion/src/scalar.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9zY2FsYXIucnM=) | `53.65% <33.33%> (-0.17%)` | :arrow_down: |
| [.../datafusion/src/physical\_plan/regex\_expressions.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3JlZ2V4X2V4cHJlc3Npb25zLnJz) | `86.15% <60.00%> (-2.18%)` | :arrow_down: |
| [rust/datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2Z1bmN0aW9ucy5ycw==) | `89.48% <90.19%> (+0.02%)` | :arrow_up: |
| [rust/arrow/src/compute/kernels/regexp.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvY29tcHV0ZS9rZXJuZWxzL3JlZ2V4cC5ycw==) | `97.46% <97.46%> (ø)` | |
| [rust/datafusion/tests/sql.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3Rlc3RzL3NxbC5ycw==) | `98.47% <100.00%> (+0.01%)` | :arrow_up: |
| [rust/arrow/src/util/display.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvdXRpbC9kaXNwbGF5LnJz) | `79.16% <0.00%> (+2.77%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=footer). Last update [894dd17...077a7dc](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-793074013
Hi @sweb
Yesterday I made my PR for `regexp_replace` (and others) see https://github.com/apache/arrow/pull/9654/files#diff-c122a83600dc86aa69067fdfdca1e0349616dfae73eaad8a8c90d5e69dbf7a3c You can see how I am passing in the values. I think there is opportunity to use more `lazy_static` to precompile any standard regex in that file to reduce runtime cost - but we need to see all the functions before too much optimisation.
The way I have done the `regexp_replace` code (and I am not saying it is the best way) is that because potentially each row can be different (as any argument to a function in Postgres can actually be supplied by referencing column) I have tried to balance that cost by memoizing the Regex objects. I did write a lot of this code prior to @jorgecarleitao doing some large changes in the `functions.rs` relating to Scalar vs Columnar so there may be a second pass to optimise this once we get basic functionality working.
I think the `regexp_match` is the way to go (as per Postgres) which does return a list of string values. We will then need to look at the sqlparser to add the ability to 'extract' values from the list by id: `[0]` (I was thinking of doing this soon). I need to do some playing in Postgres to fully understand the behaivor (what happens if you reference a non-existent index).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-792942581
> @sweb I can help on Monday. I'm planning to raise the PR for those other regexp functions then can help work through this?
Hey @seddonm1 I have rebased my PR on the current master. My plan would be to only keep `regexp_match` and remove `regexp_extract`. However, since there are review comments that I did not address yet, I did not want to remove `regexp_extract` before I was sure that `regexp_match` is the way to go.
My main issues are:
* What is the correct way to pass the regular expression into the DataFusion function? I have seen in your PR that you treat it as a normal column (ArrayRef) and compile all distinct regular expressions and apply the relevant one. I have to admit that I did not think of this use case, but of course it is much cleaner than my assumption that it is going to be a literal, fingers crossed. However, I am not sure that I would expect this API from a kernel function, i.e. passing the regex as a `StringArray`, instead of a `&str`.
* What do you think concerning the usage of `ListArray` in `regexp_match` and just defining the return types of the corresponding DataFusion functions as `List`? I have no experience in using DataFusion (yet) so I am not sure whether this makes sense or if I have to add some kind of handling for accessing list elements to make this usable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb edited a comment on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb edited a comment on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-791491062
> @sweb what is the status of this PR? Are you blocked? If you just haven't had time or inclination to make changes, that is fine (I totally understand) I just wanted try and clear the PR queue
@alamb Sorry for the late response - I did not have enough time to continue with this and did not see your message. However, I am currently blocked on one front:
I have created a new kernel function with the following signature:
```
fn regexp_match(array: &Array, pattern: &str) -> Result<ArrayRef>
```
where `ArrayRef` is a `GenericListArray`. This is my attempt to get closer to the Postgres functions. However, I do not seem to understand, how I can define a return type appropriate for `ListArray` for a DataFusion function. To be more specific: When I try to define the return type of `DataType::List` in `functions.rs` I need to create a `Box<Field>` and I am not sure whether it makes sense to create it here - e.g. what should be the name of the field?
If you have some pointers for this, I would be very grateful - I was not able to find something similar yet. I have added my current state, which kind of works but I am not too sure that what I am doing there is correct. Again, apologies for the late response.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-774703945
@jorgecarleitao could I ask you for a review if you find the time? This is my first attempt to do something with datafusion, so there are probably some things I misunderstood.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-777056311
> Given we are aiming for Postgres compatability (in terms of syntax) do you think you could modify it to be the `regexp_match ( string text, pattern text [, flags text ] ) → text[]` function instead? Then you could modify the parser to support extracting a value from the `text[]` array like Postgres.
@seddonm1 Sure, I will try to change it accordingly!
> I have done a lot of work recently on Postgres functions so there may be some useful work there: #9243
What a lucky coincidence that you have not implemented `regexp_match` yet :) I will check your PR for some pointers on how to provide things like flags, etc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-811411953
> @sweb This is great and does work with all the cases I have run tests for 👍. You can see my slight suggested change above and once thats done @alamb this is ready for merge.
>
> Sorry for being pedantic but I think this is really important functionality.
No worries, thank you for your thorough review work. I learned a lot!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-777044112
@sweb This is cool and useful.
Given we are aiming for Postgres compatability (in terms of syntax) do you think you could modify it to be the `regexp_match ( string text, pattern text [, flags text ] ) → text[]` function instead? Then you could modify the parser to support extracting a value from the `text[]` array like Postgres.
See: https://www.postgresql.org/docs/13/functions-string.html
I have done a lot of work recently on Postgres functions so there may be some useful work there: https://github.com/apache/arrow/pull/9243
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] codecov-io edited a comment on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-808895199
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=h1) Report
> Merging [#9428](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=desc) (da33017) into [master](https://codecov.io/gh/apache/arrow/commit/894dd17c9602439c2b84c0b849fb0966606ceb1c?el=desc) (894dd17) will **increase** coverage by `0.03%`.
> The diff coverage is `92.90%`.
[](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9428 +/- ##
==========================================
+ Coverage 82.42% 82.45% +0.03%
==========================================
Files 252 253 +1
Lines 58977 59132 +155
==========================================
+ Hits 48609 48755 +146
- Misses 10368 10377 +9
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `83.39% <ø> (ø)` | |
| [rust/datafusion/src/scalar.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9zY2FsYXIucnM=) | `53.65% <33.33%> (-0.17%)` | :arrow_down: |
| [.../datafusion/src/physical\_plan/regex\_expressions.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3JlZ2V4X2V4cHJlc3Npb25zLnJz) | `86.15% <60.00%> (-2.18%)` | :arrow_down: |
| [rust/datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2Z1bmN0aW9ucy5ycw==) | `89.48% <90.19%> (+0.02%)` | :arrow_up: |
| [rust/arrow/src/compute/kernels/regexp.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvY29tcHV0ZS9rZXJuZWxzL3JlZ2V4cC5ycw==) | `97.59% <97.59%> (ø)` | |
| [rust/datafusion/tests/sql.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3Rlc3RzL3NxbC5ycw==) | `98.47% <100.00%> (+0.01%)` | :arrow_up: |
| [rust/datafusion/examples/simple\_udaf.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL2V4YW1wbGVzL3NpbXBsZV91ZGFmLnJz) | | |
| [rust/datafusion/examples/flight\_server.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL2V4YW1wbGVzL2ZsaWdodF9zZXJ2ZXIucnM=) | | |
| [rust/datafusion-examples/examples/flight\_server.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uLWV4YW1wbGVzL2V4YW1wbGVzL2ZsaWdodF9zZXJ2ZXIucnM=) | `0.00% <0.00%> (ø)` | |
| ... and [2 more](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=footer). Last update [894dd17...da33017](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-811843190
Checking this out @sweb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-794523002
@seddonm1 I have pushed the following adjustment:
```
// from functions.rs
BuiltinScalarFunction::RegexpMatch => |args| match (&args[0], &args[1]) {
(c, ColumnarValue::Scalar(ScalarValue::Utf8(Some(pattern))))
if c.data_type() == DataType::Utf8
|| c.data_type() == DataType::LargeUtf8 =>
{
let arr = match c {
ColumnarValue::Array(col) => col.clone(),
ColumnarValue::Scalar(_) => c.clone().into_array(1),
};
Ok(ColumnarValue::Array(string_expressions::regexp_match(
&arr,
pattern.as_str(),
)?))
}
...
```
This way, I define that the regular expression needs to come from a literal, so I do not have to take the first element of the array and hope for the best and do not have to check for multiple regular expressions. This comes with a change I am not sure is wanted: The string_expressions function `regexp_match` has two arguments and not a single slice of ArrayRefs.
I am just proposing this because I am not sure that we want to handle multiple regular expressions. If supporting multiple expressions is the way to go, I will adjust the kernel the way you implemented regexp_replace. Let me know what you think!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-807465721
@sweb i have had a read through the code and it looks good. Are you able to rebase so the tests will be able to run? Sorry, I missed your comment 10 days ago to have a look earlier 🤦
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r588465516
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ Array, ArrayRef, GenericStringArray, GenericStringBuilder, LargeStringArray,
+ StringArray, StringOffsetSizeTrait,
+};
+use crate::datatypes::DataType;
+use crate::error::{ArrowError, Result};
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+fn generic_regexp_extract<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ re: &Regex,
+ idx: usize,
+) -> Result<ArrayRef> {
+ let mut builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+
+ for maybe_value in array.iter() {
+ match maybe_value {
+ Some(value) => match re.captures(value) {
+ Some(caps) => {
+ let m = caps.get(idx).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Regexp has no group with index {}",
+ idx
+ ))
+ })?;
+ builder.append_value(m.as_str())?
+ }
+ None => builder.append_null()?,
+ },
+ None => builder.append_null()?,
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
+/// Extracts a specific group matched by a regular expression for a given String array.
+/// Group index 0 returns the whole match, index 1 returns the first group and so on. Please
+/// refer to regex crate for details on pattern specifics.
+pub fn regexp_extract(array: &Array, pattern: &str, idx: usize) -> Result<ArrayRef> {
+ let re = Regex::new(pattern).map_err(|e| {
+ ArrowError::ComputeError(format!("Regular expression did not compile: {:?}", e))
+ })?;
+ match array.data_type() {
+ DataType::LargeUtf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<LargeStringArray>()
+ .expect("A large string is expected"),
+ &re,
+ idx,
+ ),
+ DataType::Utf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .expect("A string is expected"),
+ &re,
+ idx,
+ ),
+ _ => Err(ArrowError::ComputeError(format!(
+ "regexp_extract does not support type {:?}",
+ array.data_type()
+ ))),
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn extract_single_group() -> Result<()> {
+ let values = vec!["abc-005-def", "X-7-5", "X545"];
Review comment:
I added a `None` case for `regexp_match` - I am currently planning to remove `regexp_extract` if something like `regexp_match` is preferable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-810168087
❤️ --- thanks @sweb and @seddonm1 . I'll keep an eye on this PR and merge when ready. I took a quick skim through this morning and it is looking very nice 👌
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] codecov-io edited a comment on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-808895199
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=h1) Report
> Merging [#9428](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=desc) (f7e1745) into [master](https://codecov.io/gh/apache/arrow/commit/894dd17c9602439c2b84c0b849fb0966606ceb1c?el=desc) (894dd17) will **increase** coverage by `0.03%`.
> The diff coverage is `92.90%`.
[](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9428 +/- ##
==========================================
+ Coverage 82.42% 82.45% +0.03%
==========================================
Files 252 253 +1
Lines 58977 59132 +155
==========================================
+ Hits 48609 48755 +146
- Misses 10368 10377 +9
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `83.39% <ø> (ø)` | |
| [rust/datafusion/src/scalar.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9zY2FsYXIucnM=) | `53.65% <33.33%> (-0.17%)` | :arrow_down: |
| [.../datafusion/src/physical\_plan/regex\_expressions.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3JlZ2V4X2V4cHJlc3Npb25zLnJz) | `86.15% <60.00%> (-2.18%)` | :arrow_down: |
| [rust/datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2Z1bmN0aW9ucy5ycw==) | `89.48% <90.19%> (+0.02%)` | :arrow_up: |
| [rust/arrow/src/compute/kernels/regexp.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvY29tcHV0ZS9rZXJuZWxzL3JlZ2V4cC5ycw==) | `97.59% <97.59%> (ø)` | |
| [rust/datafusion/tests/sql.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3Rlc3RzL3NxbC5ycw==) | `98.47% <100.00%> (+0.01%)` | :arrow_up: |
| [rust/arrow/src/util/display.rs](https://codecov.io/gh/apache/arrow/pull/9428/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvdXRpbC9kaXNwbGF5LnJz) | `79.16% <0.00%> (+2.77%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=footer). Last update [894dd17...f7e1745](https://codecov.io/gh/apache/arrow/pull/9428?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r577298730
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ Array, ArrayRef, GenericStringArray, GenericStringBuilder, LargeStringArray,
+ StringArray, StringOffsetSizeTrait,
+};
+use crate::datatypes::DataType;
+use crate::error::{ArrowError, Result};
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+fn generic_regexp_extract<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ re: &Regex,
+ idx: usize,
+) -> Result<ArrayRef> {
+ let mut builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+
+ for maybe_value in array.iter() {
+ match maybe_value {
+ Some(value) => match re.captures(value) {
+ Some(caps) => {
+ let m = caps.get(idx).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Regexp has no group with index {}",
+ idx
+ ))
+ })?;
+ builder.append_value(m.as_str())?
+ }
+ None => builder.append_null()?,
+ },
+ None => builder.append_null()?,
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
+/// Extracts a specific group matched by a regular expression for a given String array.
+/// Group index 0 returns the whole match, index 1 returns the first group and so on. Please
+/// refer to regex crate for details on pattern specifics.
+pub fn regexp_extract(array: &Array, pattern: &str, idx: usize) -> Result<ArrayRef> {
+ let re = Regex::new(pattern).map_err(|e| {
+ ArrowError::ComputeError(format!("Regular expression did not compile: {:?}", e))
+ })?;
+ match array.data_type() {
+ DataType::LargeUtf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<LargeStringArray>()
+ .expect("A large string is expected"),
+ &re,
+ idx,
+ ),
+ DataType::Utf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .expect("A string is expected"),
+ &re,
+ idx,
+ ),
+ _ => Err(ArrowError::ComputeError(format!(
+ "regexp_extract does not support type {:?}",
+ array.data_type()
+ ))),
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn extract_single_group() -> Result<()> {
+ let values = vec!["abc-005-def", "X-7-5", "X545"];
+ let array = StringArray::from(values);
+ let pattern = r".*-(\d*)-.*";
+ let actual = regexp_extract(&array, pattern, 1)?;
+ let expected = StringArray::from(vec![Some("005"), Some("7"), None]);
Review comment:
if we want to mimic spark, the last entry should result in an empty string, not a `None`. This is because it would be otherwise impossible to differentiate between a "no match" and a "input is null".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r589648321
##########
File path: rust/datafusion/src/physical_plan/functions.rs
##########
@@ -523,6 +544,7 @@ fn signature(fun: &BuiltinScalarFunction) -> Signature {
BuiltinScalarFunction::NullIf => {
Signature::Uniform(2, SUPPORTED_NULLIF_TYPES.to_vec())
}
+ BuiltinScalarFunction::RegexpExtract => Signature::Any(3),
Review comment:
Done - I had to use `Int64` instead of `UInt64` because I got errors from `tests/sql.rs`. I have to check how to define a literal as unsigned from within the query string.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-795695634
I have pushed a first version of `regexp_match` supporting literals and columns as pattern. Currently unfinished:
* [ ] Having both string and pattern as literal is currently failing, because `List` is not supported as a scalar return - I will try to fix this
* [ ] Flags are missing
* [ ] the kernel function is quite messy - I will try to see what is required to refactor it as iterator chains
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-793109365
> Again, apologies for the late response.
@sweb no worries! I *totally* understand. Thanks for sticking with it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-795012431
@sweb I know you have spent a lot of time on this so I know its painful but I do think that we should built to the Postgres spec which will allow regex to be passed in via a columnar value. How we implement that behind the scene allows optimisation but the spec allows each row to be individually processed with a different regex.
To clarify: I am not sure if anyone _actually_ uses SQL like this but it is a consistent implementation pattern in Postgres where a Scalar and Columnar value is treated equally - which I think is an elegant design.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-811857097
I also merged it with apache/master locally and ran the tests / clippy and everything passed for me. Thus merging this one in
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r603655693
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ ArrayRef, GenericStringArray, GenericStringBuilder, ListBuilder,
+ StringOffsetSizeTrait,
+};
+use crate::error::{ArrowError, Result};
+use std::collections::HashMap;
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+/// Extract all groups matched by a regular expression for a given String array.
+pub fn regexp_match<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ regex_array: &GenericStringArray<OffsetSize>,
+ flags_array: Option<&GenericStringArray<OffsetSize>>,
+) -> Result<ArrayRef> {
+ let mut patterns: HashMap<String, Regex> = HashMap::new();
+ let builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+ let mut list_builder = ListBuilder::new(builder);
+
+ let complete_pattern = match flags_array {
+ Some(flags) => Box::new(regex_array.iter().zip(flags.iter()).map(
+ |(pattern, flags)| {
+ pattern.map(|pattern| match flags {
+ Some(value) => format!("(?{}){}", value, pattern),
+ None => pattern.to_string(),
+ })
+ },
+ )) as Box<dyn Iterator<Item = Option<String>>>,
+ None => Box::new(
+ regex_array
+ .iter()
+ .map(|pattern| pattern.map(|pattern| pattern.to_string())),
+ ),
+ };
+ array
+ .iter()
+ .zip(complete_pattern)
+ .map(|(value, pattern)| {
+ match (value, pattern) {
+ (Some(value), Some(pattern)) => {
+ let existing_pattern = patterns.get(&pattern);
+ let re = match existing_pattern {
+ Some(re) => re.clone(),
+ None => {
+ let re = Regex::new(pattern.as_str()).map_err(|e| {
+ ArrowError::ComputeError(format!(
+ "Regular expression did not compile: {:?}",
+ e
+ ))
+ })?;
+ patterns.insert(pattern, re.clone());
+ re
+ }
+ };
+ match re.captures(value) {
+ Some(caps) => {
+ for m in caps.iter().skip(1) {
+ if let Some(v) = m {
+ list_builder.values().append_value(v.as_str())?;
+ }
+ }
+ list_builder.append(true)?
+ }
+ None => {
+ list_builder.values().append_value("")?;
+ list_builder.append(true)?
+ }
+ }
+ }
+ _ => list_builder.append(false)?,
+ }
+ Ok(())
+ })
+ .collect::<Result<Vec<()>>>()?;
+ Ok(Arc::new(list_builder.finish()))
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::array::{ListArray, StringArray};
+
+ #[test]
+ fn match_single_group() -> Result<()> {
Review comment:
I did not know about this case. I just put a few scenarios into a Postgres instance running locally (via docker). Your implementation does make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r577945055
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ Array, ArrayRef, GenericStringArray, GenericStringBuilder, LargeStringArray,
+ StringArray, StringOffsetSizeTrait,
+};
+use crate::datatypes::DataType;
+use crate::error::{ArrowError, Result};
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+fn generic_regexp_extract<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ re: &Regex,
+ idx: usize,
+) -> Result<ArrayRef> {
+ let mut builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+
+ for maybe_value in array.iter() {
+ match maybe_value {
+ Some(value) => match re.captures(value) {
+ Some(caps) => {
+ let m = caps.get(idx).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Regexp has no group with index {}",
+ idx
+ ))
+ })?;
+ builder.append_value(m.as_str())?
+ }
+ None => builder.append_null()?,
+ },
+ None => builder.append_null()?,
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
+/// Extracts a specific group matched by a regular expression for a given String array.
+/// Group index 0 returns the whole match, index 1 returns the first group and so on. Please
+/// refer to regex crate for details on pattern specifics.
+pub fn regexp_extract(array: &Array, pattern: &str, idx: usize) -> Result<ArrayRef> {
Review comment:
Hey @jorgecarleitao thank you very much for your review. I will try to address your comments in the next days.
Since @seddonm1 remarked that Postgres compatibility, I was thinking about changing the function signature of the kernel to:
```
pub fn regexp_match(array: &Array, pattern: &str) -> Result<ArrayRef>
```
where the returned array is of type `GenericListArray` with values of type `&str`. A list is closer to the Postgres signature and would provide the flexibility to choose multiple groups. Would this be fine as well or is `Vec<ArrayRef>` preferable to you?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-805099807
@seddonm1 could you take another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9428: ARROW-10354: [Rust] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-774180357
https://issues.apache.org/jira/browse/ARROW-10354
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-808554523
@sweb This looks good and I think it is almost ready. I think there are just a few cases to look into to match Postgres behavior then ready to merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] seddonm1 commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r602608614
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ ArrayRef, GenericStringArray, GenericStringBuilder, ListBuilder,
+ StringOffsetSizeTrait,
+};
+use crate::error::{ArrowError, Result};
+use std::collections::HashMap;
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+/// Extract all groups matched by a regular expression for a given String array.
+pub fn regexp_match<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ regex_array: &GenericStringArray<OffsetSize>,
+ flags_array: Option<&GenericStringArray<OffsetSize>>,
+) -> Result<ArrayRef> {
+ let mut patterns: HashMap<String, Regex> = HashMap::new();
+ let builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+ let mut list_builder = ListBuilder::new(builder);
+
+ let complete_pattern = match flags_array {
+ Some(flags) => Box::new(regex_array.iter().zip(flags.iter()).map(
+ |(pattern, flags)| {
+ pattern.map(|pattern| match flags {
+ Some(value) => format!("(?{}){}", value, pattern),
+ None => pattern.to_string(),
+ })
+ },
+ )) as Box<dyn Iterator<Item = Option<String>>>,
+ None => Box::new(
+ regex_array
+ .iter()
+ .map(|pattern| pattern.map(|pattern| pattern.to_string())),
+ ),
+ };
+ array
+ .iter()
+ .zip(complete_pattern)
+ .map(|(value, pattern)| {
+ match (value, pattern) {
+ (Some(value), Some(pattern)) => {
+ let existing_pattern = patterns.get(&pattern);
+ let re = match existing_pattern {
+ Some(re) => re.clone(),
+ None => {
+ let re = Regex::new(pattern.as_str()).map_err(|e| {
+ ArrowError::ComputeError(format!(
+ "Regular expression did not compile: {:?}",
+ e
+ ))
+ })?;
+ patterns.insert(pattern, re.clone());
+ re
+ }
+ };
+ match re.captures(value) {
+ Some(caps) => {
+ for m in caps.iter().skip(1) {
+ if let Some(v) = m {
+ list_builder.values().append_value(v.as_str())?;
+ }
+ }
+ list_builder.append(true)?
+ }
+ None => {
+ list_builder.values().append_value("")?;
Review comment:
I am not sure this logic is correct based on Postgres behavior:
```sql
SELECT regexp_match('foobarbequebaz', '(bar)(bequ1e)') = NULL
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-808496592
@seddonm1 I just merged master - there is currently a linting issue due to rust 1.51 but the tests are green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on a change in pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#discussion_r588464398
##########
File path: rust/datafusion/src/physical_plan/string_expressions.rs
##########
@@ -70,6 +71,16 @@ pub fn concatenate(args: &[ArrayRef]) -> Result<StringArray> {
Ok(builder.finish())
}
+/// extract a specific group from a string column, using a regular expression
+pub fn regexp_extract(args: &[ArrayRef]) -> Result<ArrayRef> {
+ let pattern_expr = args[1].as_any().downcast_ref::<StringArray>().unwrap();
+ let pattern = pattern_expr.value(0);
+ let idx_expr = args[2].as_any().downcast_ref::<Int64Array>().unwrap();
+ let idx = idx_expr.value(0) as usize;
Review comment:
Absolutely, I did this because I am not sure, how I can get from `ArrayRef` to `ScalarVariant` - I am looking for something like the following:
```
let pattern_expr = args[1].as_any().downcast_ref::<ScalarValue>().unwrap();
if let ScalarValue::Utf8(Some(pattern)) = pattern_expr {
compute::regexp_match(args[0].as_ref(), pattern)
.map_err(DataFusionError::ArrowError)
} else {
Err(DataFusionError::Internal("This is wrong".to_string()))
}
```
##########
File path: rust/arrow/src/compute/kernels/regexp.rs
##########
@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines kernel to extract substrings based on a regular
+//! expression of a \[Large\]StringArray
+
+use crate::array::{
+ Array, ArrayRef, GenericStringArray, GenericStringBuilder, LargeStringArray,
+ StringArray, StringOffsetSizeTrait,
+};
+use crate::datatypes::DataType;
+use crate::error::{ArrowError, Result};
+
+use std::sync::Arc;
+
+use regex::Regex;
+
+fn generic_regexp_extract<OffsetSize: StringOffsetSizeTrait>(
+ array: &GenericStringArray<OffsetSize>,
+ re: &Regex,
+ idx: usize,
+) -> Result<ArrayRef> {
+ let mut builder: GenericStringBuilder<OffsetSize> = GenericStringBuilder::new(0);
+
+ for maybe_value in array.iter() {
+ match maybe_value {
+ Some(value) => match re.captures(value) {
+ Some(caps) => {
+ let m = caps.get(idx).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Regexp has no group with index {}",
+ idx
+ ))
+ })?;
+ builder.append_value(m.as_str())?
+ }
+ None => builder.append_null()?,
+ },
+ None => builder.append_null()?,
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
+/// Extracts a specific group matched by a regular expression for a given String array.
+/// Group index 0 returns the whole match, index 1 returns the first group and so on. Please
+/// refer to regex crate for details on pattern specifics.
+pub fn regexp_extract(array: &Array, pattern: &str, idx: usize) -> Result<ArrayRef> {
+ let re = Regex::new(pattern).map_err(|e| {
+ ArrowError::ComputeError(format!("Regular expression did not compile: {:?}", e))
+ })?;
+ match array.data_type() {
+ DataType::LargeUtf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<LargeStringArray>()
+ .expect("A large string is expected"),
+ &re,
+ idx,
+ ),
+ DataType::Utf8 => generic_regexp_extract(
+ array
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .expect("A string is expected"),
+ &re,
+ idx,
+ ),
+ _ => Err(ArrowError::ComputeError(format!(
+ "regexp_extract does not support type {:?}",
+ array.data_type()
+ ))),
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn extract_single_group() -> Result<()> {
+ let values = vec!["abc-005-def", "X-7-5", "X545"];
+ let array = StringArray::from(values);
+ let pattern = r".*-(\d*)-.*";
+ let actual = regexp_extract(&array, pattern, 1)?;
+ let expected = StringArray::from(vec![Some("005"), Some("7"), None]);
+ let result = actual.as_any().downcast_ref::<StringArray>().unwrap();
+ assert_eq!(&expected, result);
+ Ok(())
+ }
+
+ #[test]
+ fn no_matches() -> Result<()> {
+ let values = vec!["abc", "X::50::00", "X545"];
+ let array = StringArray::from(values);
+ let pattern = r".*-(\d*)-.*";
+ let actual = regexp_extract(&array, pattern, 1)?;
+ let expected = StringArray::from(vec![None, None, None]);
+ let result = actual.as_any().downcast_ref::<StringArray>().unwrap();
+ assert_eq!(&expected, result);
+ Ok(())
+ }
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-791491062
> @sweb what is the status of this PR? Are you blocked? If you just haven't had time or inclination to make changes, that is fine (I totally understand) I just wanted try and clear the PR queue
@alamb Sorry for the late response - I did not have enough time to continue with this and did not see your message. However, I am currently blocked on one front:
I have created a new kernel function with the following signature:
```
fn regexp_match(array: &Array, pattern: &str) -> Result<ArrayRef>
```
where `ArrayRef` is a `GenericListArray`. This is my attempt to get closer to the Postgres functions. However, I do not seem to understand, how I can define a return type appropriate for `ListArray` for a DataFusion function. To be more specific: When I try to define the return type of `DataType::List` in `functions.rs` I need to create a `Box<Field>` and I am not sure whether it makes sense to create it here - e.g. what should be the name of the field?
If you have some pointers for this, I would be very grateful - I was not able to find something similar yet. Again, apologies for the late response.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #9428: ARROW-10354: [Rust][DataFusion] regexp_extract function to select regex groups from strings
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #9428:
URL: https://github.com/apache/arrow/pull/9428#issuecomment-811749756
I merged master, but I still get (as far as I can tell) unrelated linting errors. I assume #9867 solves this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org