Add support for starts with and ends with to string filter (#11341)

Author: abey79

crates/viewer/re_dataframe_ui/src/filters/boolean.rs

@@ -51,12 +51,7 @@ impl NonNullableBooleanFilter {
         self.as_bool().to_string()
     }
 
-    pub fn popup_ui(
-        &mut self,
-        ui: &mut egui::Ui,
-        column_name: &str,
-        action: &mut super::FilterUiAction,
-    ) {
+    pub fn popup_ui(&mut self, ui: &mut egui::Ui, column_name: &str) -> FilterUiAction {
         super::basic_operation_ui(ui, column_name, "is");
 
         let mut clicked = false;
@@ -70,7 +65,9 @@ impl NonNullableBooleanFilter {
             .clicked();
 
         if clicked {
-            *action = FilterUiAction::CommitStateToBlueprint;
+            FilterUiAction::CommitStateToBlueprint
+        } else {
+            FilterUiAction::None
         }
     }
 }
@@ -139,12 +136,7 @@ impl NullableBooleanFilter {
         }
     }
 
-    pub fn popup_ui(
-        &mut self,
-        ui: &mut egui::Ui,
-        column_name: &str,
-        action: &mut super::FilterUiAction,
-    ) {
+    pub fn popup_ui(&mut self, ui: &mut egui::Ui, column_name: &str) -> FilterUiAction {
         super::basic_operation_ui(ui, column_name, "is");
 
         let mut clicked = false;
@@ -160,7 +152,9 @@ impl NullableBooleanFilter {
             .clicked();
 
         if clicked {
-            *action = FilterUiAction::CommitStateToBlueprint;
+            FilterUiAction::CommitStateToBlueprint
+        } else {
+            FilterUiAction::None
         }
     }
 }

crates/viewer/re_dataframe_ui/src/filters/filter.rs

@@ -12,13 +12,14 @@ use datafusion::logical_expr::{
     ArrayFunctionArgument, ArrayFunctionSignature, ColumnarValue, ScalarFunctionArgs, ScalarUDF,
     ScalarUDFImpl, Signature, TypeSignature, Volatility,
 };
-use datafusion::prelude::{Column, Expr, array_to_string, col, contains, lit, lower};
+use datafusion::prelude::{Column, Expr, col};
 
 use re_types_core::datatypes::TimeInt;
 use re_types_core::{Component as _, FIELD_METADATA_KEY_COMPONENT_TYPE, Loggable as _};
 
 use super::{
-    FloatFilter, IntFilter, NonNullableBooleanFilter, NullableBooleanFilter, TimestampFilter,
+    FloatFilter, IntFilter, NonNullableBooleanFilter, NullableBooleanFilter, StringFilter,
+    TimestampFilter, is_supported_string_datatype,
 };
 
 /// The nullability of a nested arrow datatype.
@@ -92,14 +93,14 @@ pub enum FilterError {
     #[error("column {0} was not found")]
     ColumnNotFound(Column),
 
-    #[error("invalid filter kind {0:?} for field {1}")]
-    InvalidFilterKind(Box<FilterKind>, Box<Field>),
-
     #[error("invalid non-nullable boolean filter {0:?} for field {1}")]
     InvalidNonNullableBooleanFilter(NonNullableBooleanFilter, Box<Field>),
 
     #[error("invalid nullable boolean filter {0:?} for field {1}")]
     InvalidNullableBooleanFilter(NullableBooleanFilter, Box<Field>),
+
+    #[error("invalid string filter {0:?} for field {1}")]
+    InvalidStringFilter(StringFilter, Box<Field>),
 }
 
 /// A filter applied to a table.
@@ -137,10 +138,7 @@ pub enum FilterKind {
     NonNullableBoolean(NonNullableBooleanFilter),
     Int(IntFilter),
     Float(FloatFilter),
-
-    //TODO(ab): parameterise that over multiple string ops, e.g. "contains", "starts with", etc.
-    StringContains(String),
-
+    String(StringFilter),
     Timestamp(TimestampFilter),
 }
 
@@ -194,9 +192,11 @@ impl FilterKind {
                 Some(Self::Float(Default::default()))
             }
 
-            DataType::Utf8 | DataType::Utf8View => Some(Self::StringContains(String::new())),
+            data_type if is_supported_string_datatype(data_type) => {
+                Some(Self::String(Default::default()))
+            }
 
-            DataType::Timestamp(_, _) => Some(Self::Timestamp(TimestampFilter::default())),
+            DataType::Timestamp(_, _) => Some(Self::Timestamp(Default::default())),
 
             _ => None,
         }
@@ -225,35 +225,7 @@ impl FilterKind {
                 Ok(udf.call(vec![col(column.clone())]))
             }
 
-            Self::StringContains(query_string) => {
-                if query_string.is_empty() {
-                    return Ok(lit(true));
-                }
-
-                let operand = match field.data_type() {
-                    DataType::Utf8 | DataType::Utf8View => col(column.clone()),
-
-                    DataType::List(field) | DataType::ListView(field)
-                        if field.data_type() == &DataType::Utf8
-                            || field.data_type() == &DataType::Utf8View =>
-                    {
-                        // For List[Utf8], we concatenate all the instances into a single logical
-                        // string, separated by a Record Separator (0x1E) character. This ensures
-                        // that the query string doesn't accidentally match a substring spanning
-                        // multiple instances.
-                        array_to_string(col(column.clone()), lit("\u{001E}"))
-                    }
-
-                    _ => {
-                        return Err(FilterError::InvalidFilterKind(
-                            self.clone().into(),
-                            field.clone().into(),
-                        ));
-                    }
-                };
-
-                Ok(contains(lower(operand), lower(lit(query_string))))
-            }
+            Self::String(string_filter) => Ok(string_filter.as_filter_expression(column)),
         }
     }
 }
@@ -442,6 +414,7 @@ impl FilterKindUdf {
         // 1) Convert the list array to a bool array (with same offsets and nulls)
         // 2) Apply the ANY (or, in the future, another) semantics to "merge" each row's instances
         //    into the final bool.
+        // TODO(ab): duplicated code with the other UDF, pliz unify.
         cast_list_array
             .iter()
             .map(|maybe_row| {

crates/viewer/re_dataframe_ui/src/filters/filter_ui.rs

@@ -315,6 +315,7 @@ impl Filter {
 
 impl SyntaxHighlighting for TimestampFormatted<'_, FilterKind> {
     fn syntax_highlight_into(&self, builder: &mut SyntaxHighlightedBuilder) {
+        //TODO(ab): this is weird. this entire impl should be delegated to inner structs
         builder.append_keyword(&self.inner.operator_text());
         builder.append_keyword(" ");
 
@@ -335,8 +336,8 @@ impl SyntaxHighlighting for TimestampFormatted<'_, FilterKind> {
                 builder.append(float_filter);
             }
 
-            FilterKind::StringContains(query) => {
-                builder.append_string_value(query);
+            FilterKind::String(string_filter) => {
+                builder.append(string_filter);
             }
 
             FilterKind::Timestamp(timestamp_filter) => {
@@ -364,51 +365,22 @@ impl FilterKind {
         column_name: &str,
         popup_just_opened: bool,
     ) -> FilterUiAction {
-        let mut action = FilterUiAction::None;
-
-        let operator_text = self.operator_text();
-
         // Reduce the default width unnecessarily expands the popup width (queries as usually vers
         // small).
         ui.spacing_mut().text_edit_width = 150.0;
 
-        // TODO(ab): this is getting unwieldy. All arms should have an independent inner struct,
-        // which all handle their own UI.
         match self {
-            Self::NonNullableBoolean(boolean_filter) => {
-                boolean_filter.popup_ui(ui, column_name, &mut action);
-            }
-
-            Self::NullableBoolean(boolean_filter) => {
-                boolean_filter.popup_ui(ui, column_name, &mut action);
-            }
-
-            Self::Int(int_filter) => {
-                action = int_filter.popup_ui(ui, column_name, popup_just_opened);
-            }
-
-            Self::Float(float_filter) => {
-                action = float_filter.popup_ui(ui, column_name, popup_just_opened);
-            }
-
-            Self::StringContains(query) => {
-                basic_operation_ui(ui, column_name, &operator_text);
-
-                let response = ui.text_edit_singleline(query);
-
-                if popup_just_opened {
-                    response.request_focus();
-                }
-
-                action = action_from_text_edit_response(ui, &response);
+            Self::NonNullableBoolean(boolean_filter) => boolean_filter.popup_ui(ui, column_name),
+            Self::NullableBoolean(boolean_filter) => boolean_filter.popup_ui(ui, column_name),
+            Self::Int(int_filter) => int_filter.popup_ui(ui, column_name, popup_just_opened),
+            Self::Float(float_filter) => float_filter.popup_ui(ui, column_name, popup_just_opened),
+            Self::String(string_filter) => {
+                string_filter.popup_ui(ui, column_name, popup_just_opened)
             }
-
             Self::Timestamp(timestamp_filter) => {
-                timestamp_filter.popup_ui(ui, column_name, &mut action, timestamp_format);
+                timestamp_filter.popup_ui(ui, column_name, timestamp_format)
             }
         }
-
-        action
     }
 
     /// Given a chance to the underlying filter struct to update/clean itself upon committing the
@@ -422,7 +394,7 @@ impl FilterKind {
             | Self::NonNullableBoolean(_)
             | Self::Int(_)
             | Self::Float(_)
-            | Self::StringContains(_) => {}
+            | Self::String(_) => {}
 
             Self::Timestamp(timestamp_filter) => timestamp_filter.on_commit(),
         }
@@ -433,7 +405,7 @@ impl FilterKind {
         match self {
             Self::Int(int_filter) => int_filter.comparison_operator().to_string(),
             Self::Float(float_filter) => float_filter.comparison_operator().to_string(),
-            Self::StringContains(_) => "contains".to_owned(),
+            Self::String(string_filter) => string_filter.operator().to_string(),
             Self::NonNullableBoolean(_) | Self::NullableBoolean(_) | Self::Timestamp(_) => {
                 "is".to_owned()
             }
@@ -462,22 +434,23 @@ pub fn action_from_text_edit_response(ui: &egui::Ui, response: &egui::Response)
 mod tests {
     use super::super::{
         ComparisonOperator, FloatFilter, IntFilter, NonNullableBooleanFilter,
-        NullableBooleanFilter, TimestampFilter,
+        NullableBooleanFilter, StringFilter, TimestampFilter,
     };
     use super::*;
+    use crate::filters::StringOperator;
 
     fn test_cases() -> Vec<(FilterKind, &'static str)> {
         // Let's remember to update this test when adding new filter operations.
         #[cfg(debug_assertions)]
         let _: () = {
             use FilterKind::*;
-            let _op = StringContains(String::new());
+            let _op = String(Default::default());
             match _op {
                 NonNullableBoolean(_)
                 | NullableBoolean(_)
                 | Int(_)
                 | Float(_)
-                | StringContains(_)
+                | String(_)
                 | Timestamp(_) => {}
             }
         };
@@ -520,13 +493,17 @@ mod tests {
                 "float_compares_none",
             ),
             (
-                FilterKind::StringContains("query".to_owned()),
+                FilterKind::String(StringFilter::new(StringOperator::Contains, "query")),
                 "string_contains",
             ),
             (
-                FilterKind::StringContains(String::new()),
+                FilterKind::String(StringFilter::new(StringOperator::Contains, "")),
                 "string_contains_empty",
             ),
+            (
+                FilterKind::String(StringFilter::new(StringOperator::StartsWith, "query")),
+                "string_starts_with",
+            ),
             (
                 FilterKind::Timestamp(TimestampFilter::after(
                     jiff::Timestamp::from_millisecond(100_000_000_000).unwrap(),
@@ -607,18 +584,39 @@ mod tests {
         let filters = vec![
             Filter::new(
                 "some:column:name",
-                FilterKind::StringContains("some query string".to_owned()),
+                FilterKind::String(StringFilter::new(
+                    StringOperator::Contains,
+                    "some query string".to_owned(),
+                )),
             ),
             Filter::new(
                 "other:column:name",
-                FilterKind::StringContains("hello".to_owned()),
+                FilterKind::String(StringFilter::new(
+                    StringOperator::Contains,
+                    "hello".to_owned(),
+                )),
+            ),
+            Filter::new(
+                "short:name",
+                FilterKind::String(StringFilter::new(
+                    StringOperator::Contains,
+                    "world".to_owned(),
+                )),
             ),
-            Filter::new("short:name", FilterKind::StringContains("world".to_owned())),
             Filter::new(
                 "looooog:name",
-                FilterKind::StringContains("some more querying text here".to_owned()),
+                FilterKind::String(StringFilter::new(
+                    StringOperator::Contains,
+                    "some more querying text here".to_owned(),
+                )),
+            ),
+            Filter::new(
+                "world",
+                FilterKind::String(StringFilter::new(
+                    StringOperator::Contains,
+                    ":wave:".to_owned(),
+                )),
             ),
-            Filter::new("world", FilterKind::StringContains(":wave:".to_owned())),
         ];
 
         let mut filters = FilterState {

crates/viewer/re_dataframe_ui/src/filters/mod.rs

@@ -3,10 +3,11 @@ mod filter;
 mod filter_ui;
 mod numerical;
 mod parse_timestamp;
+mod string;
 mod timestamp;
 mod timestamp_formatted;
 
 pub use self::{
-    boolean::*, filter::*, filter_ui::*, numerical::*, parse_timestamp::*, timestamp::*,
+    boolean::*, filter::*, filter_ui::*, numerical::*, parse_timestamp::*, string::*, timestamp::*,
     timestamp_formatted::*,
 };