Fix how quickwit handles "." in field names. (#1362)

After this PR we still accept field names with a ".".
On the query side this dot then needs to be escaped with a backslash
to disambiguate it from the "." of Json fields, and the "." of
object fields.

Under the hood, we still rely on tantivy's query parser.
The hack consists in escaping the tantivy field name itself.

This PR updates tantivy in order to include
quickwit-oss/tantivy#1356.

Closes #1338
Closes #1334

Author: fulmicoton

Cargo.lock

@@ -25,7 +25,7 @@ quickwit-config = { version = "0.2.1", path = "../quickwit-config" }
 tokio = { version = "1", features = ["full"] }
 tokio-util = { version = "0.7", features = ["full"] }
 rand = "0.8"
-tantivy = { git = "https://github.com/quickwit-oss/tantivy", rev = "8c1e1cf", default-features = false, features = [
+tantivy = { git = "https://github.com/quickwit-oss/tantivy", rev = "ed26552", default-features = false, features = [
     "mmap",
     "lz4-compression",
     "quickwit",

quickwit-core/Cargo.toml

@@ -16,7 +16,7 @@ futures = "0.3"
 serde = "1"
 serde_cbor = "0.11"
 serde_json = "1"
-tantivy = { git = "https://github.com/quickwit-oss/tantivy", rev = "8c1e1cf", default-features = false, features = [
+tantivy = { git = "https://github.com/quickwit-oss/tantivy", rev = "ed26552", default-features = false, features = [
     "mmap",
     "lz4-compression",
     "quickwit",

quickwit-directories/Cargo.toml

@@ -18,8 +18,8 @@ once_cell = "1.10"
 regex = "1"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
-tantivy = { git = "https://github.com/quickwit-oss/tantivy", rev = "8c1e1cf", default-features = false, features = ["mmap", "lz4-compression", "quickwit"] }
-tantivy-query-grammar = { git = "https://github.com/quickwit-oss/tantivy/", rev = "8c1e1cf" }
+tantivy = { git = "https://github.com/quickwit-oss/tantivy", rev = "ed26552", default-features = false, features = ["mmap", "lz4-compression", "quickwit"] }
+tantivy-query-grammar = { git = "https://github.com/quickwit-oss/tantivy/", rev = "ed26552" }
 thiserror = "1.0"
 tracing = "0.1.29"
 typetag = "0.1"

quickwit-doc-mapper/Cargo.toml

@@ -524,6 +524,7 @@ impl DocMapper for DefaultDocMapper {
 mod tests {
     use std::collections::HashMap;
 
+    use quickwit_proto::SearchRequest;
     use serde_json::{self, json, Value as JsonValue};
     use tantivy::schema::{FieldType, Type, Value};
 
@@ -560,9 +561,9 @@ mod tests {
             "owner": ["foo"],
             "body_other_tokenizer": ["20200415T072306-0700 INFO This is a great log"],
             "attributes.server": ["ABC"],
-            "attributes.server.payload": [[97], [98]],
+            "attributes.server\\.payload": [[97], [98]],
             "attributes.tags": [22, 23],
-            "attributes.server.status": ["200", "201"]
+            "attributes.server\\.status": ["200", "201"]
         }"#;
 
     #[test]
@@ -573,7 +574,7 @@ mod tests {
         default_search_field_names.sort();
         assert_eq!(
             default_search_field_names,
-            ["attributes.server", "attributes.server.status", "body"]
+            ["attributes.server", r#"attributes.server\.status"#, "body"]
         );
         assert_eq!(config.field_mappings.num_fields(), 9);
         Ok(())
@@ -647,19 +648,20 @@ mod tests {
     }
 
     #[test]
-    fn test_accept_parsing_document_with_unknown_fields_and_missing_fields() -> anyhow::Result<()> {
+    fn test_accept_parsing_document_with_unknown_fields_and_missing_fields() {
         let doc_mapper = crate::default_doc_mapper_for_tests();
-        doc_mapper.doc_from_json(
-            r#"{
+        doc_mapper
+            .doc_from_json(
+                r#"{
                 "timestamp": 1586960586000,
                 "unknown_field": "20200415T072306-0700 INFO This is a great log",
                 "response_date": "2021-12-19T16:39:57+00:00",
                 "response_time": 12,
                 "response_payload": "YWJj"
             }"#
-            .to_string(),
-        )?;
-        Ok(())
+                .to_string(),
+            )
+            .unwrap();
     }
 
     #[test]
@@ -1332,4 +1334,97 @@ mod tests {
             panic!("Expected json");
         }
     }
+
+    fn default_doc_mapper_query_aux(
+        doc_mapper: &dyn DocMapper,
+        query: &str,
+    ) -> Result<String, String> {
+        let search_request = SearchRequest {
+            query: query.to_string(),
+            ..Default::default()
+        };
+        let query = doc_mapper
+            .query(doc_mapper.schema(), &search_request)
+            .map_err(|err| err.to_string())?;
+        Ok(format!("{:?}", query))
+    }
+
+    #[test]
+    fn test_doc_mapper_sub_field_query_on_non_json_field_should_error() {
+        let doc_mapper: DefaultDocMapper = serde_json::from_str(
+            r#"{
+            "field_mappings": [{"name": "body", "type": "text"}],
+            "mode": "dynamic"
+        }"#,
+        )
+        .unwrap();
+        assert_eq!(
+            default_doc_mapper_query_aux(&doc_mapper, "body.wrong_field:hello").unwrap_err(),
+            "Field does not exists: 'body.wrong_field'"
+        );
+    }
+
+    #[test]
+    fn test_doc_mapper_accept_sub_field_query_on_json_field() {
+        let doc_mapper: DefaultDocMapper = serde_json::from_str(
+            r#"{
+            "field_mappings": [{"name": "body", "type": "json"}],
+            "mode": "dynamic"
+        }"#,
+        )
+        .unwrap();
+        assert_eq!(
+            default_doc_mapper_query_aux(&doc_mapper, "body.dynamic_field:hello"),
+            Ok(
+                r#"TermQuery(Term(type=Json, field=0, path=dynamic_field, vtype=Str, "hello"))"#
+                    .to_string()
+            )
+        );
+    }
+
+    #[test]
+    fn test_doc_mapper_object_dot_collision_with_object_field() {
+        let doc_mapper: DefaultDocMapper = serde_json::from_str(
+            r#"{
+            "field_mappings": [
+                {
+                    "name": "identity",
+                    "type": "object",
+                    "field_mappings": [{"type": "text", "name": "username"}]
+                },
+                {"type": "text", "name": "identity.username"}
+            ]
+        }"#,
+        )
+        .unwrap();
+        assert_eq!(
+            default_doc_mapper_query_aux(&doc_mapper, "identity.username:toto").unwrap(),
+            r#"TermQuery(Term(type=Str, field=0, "toto"))"#
+        );
+        assert_eq!(
+            default_doc_mapper_query_aux(&doc_mapper, r#"identity\.username:toto"#).unwrap(),
+            r#"TermQuery(Term(type=Str, field=1, "toto"))"#
+        );
+    }
+
+    #[test]
+    fn test_doc_mapper_object_dot_collision_with_json_field() {
+        let doc_mapper: DefaultDocMapper = serde_json::from_str(
+            r#"{
+            "field_mappings": [
+                {"name": "identity", "type": "json"},
+                {"type": "text", "name": "identity.username"}
+            ]
+        }"#,
+        )
+        .unwrap();
+        assert_eq!(
+            default_doc_mapper_query_aux(&doc_mapper, "identity.username:toto").unwrap(),
+            r#"TermQuery(Term(type=Json, field=0, path=username, vtype=Str, "toto"))"#
+        );
+        assert_eq!(
+            default_doc_mapper_query_aux(&doc_mapper, r#"identity\.username:toto"#).unwrap(),
+            r#"TermQuery(Term(type=Str, field=1, "toto"))"#
+        );
+    }
 }

quickwit-doc-mapper/src/default_doc_mapper/default_mapper.rs

@@ -21,6 +21,7 @@ use std::any::type_name;
 use std::collections::BTreeMap;
 
 use anyhow::bail;
+use itertools::Itertools;
 use serde_json::Value as JsonValue;
 use tantivy::schema::{
     BytesOptions, Cardinality, Field, JsonObjectOptions, NumericOptions, SchemaBuilder,
@@ -520,12 +521,36 @@ fn get_bytes_options(quickwit_numeric_options: &QuickwitNumericOptions) -> Bytes
     bytes_options
 }
 
+/// Creates a tantivy field name for a given field path.
+///
+/// By field path, we mean the list of `field_name` that are crossed
+/// to reach the field starting from the root of the document.
+/// There can be more than one due to quickwit object type.
+///
+/// We simply concatenate these field names, interleaving them with '.'.
+/// If a fieldname itself contains a '.', we escape it with '\'.
+/// ('\' itself is forbidden).
+fn field_name_for_field_path(field_path: &[&str]) -> String {
+    field_path.iter().cloned().map(escape_dots).join(".")
+}
+
+fn escape_dots(field_name: &str) -> String {
+    let mut escaped_field_name = String::new();
+    for chr in field_name.chars() {
+        if chr == '.' {
+            escaped_field_name.push('\\');
+        }
+        escaped_field_name.push(chr);
+    }
+    escaped_field_name
+}
+
 fn build_mapping_from_field_type<'a>(
     field_mapping_type: &'a FieldMappingType,
     field_path: &mut Vec<&'a str>,
     schema_builder: &mut SchemaBuilder,
 ) -> anyhow::Result<MappingTree> {
-    let field_name = field_path.join(".");
+    let field_name = field_name_for_field_path(field_path);
     match field_mapping_type {
         FieldMappingType::Text(options, cardinality) => {
             let text_options: TextOptions = options.clone().into();
@@ -619,6 +644,29 @@ mod tests {
         QuickwitNumericOptions, QuickwitTextOptions,
     };
 
+    #[test]
+    fn test_field_name_from_field_path() {
+        // not really a possibility, but still, let's test it.
+        assert_eq!(super::field_name_for_field_path(&[]), "");
+        assert_eq!(super::field_name_for_field_path(&["hello"]), "hello");
+        assert_eq!(
+            super::field_name_for_field_path(&["one", "two", "three"]),
+            "one.two.three"
+        );
+        assert_eq!(
+            super::field_name_for_field_path(&["one", "two", "three"]),
+            "one.two.three"
+        );
+        assert_eq!(
+            super::field_name_for_field_path(&["one.two"]),
+            r#"one\.two"#
+        );
+        assert_eq!(
+            super::field_name_for_field_path(&["one.two", "three"]),
+            r#"one\.two.three"#
+        );
+    }
+
     #[test]
     fn test_get_or_insert_path() {
         let mut map = Default::default();

quickwit-doc-mapper/src/default_doc_mapper/mapping_tree.rs

@@ -54,7 +54,7 @@ pub fn default_doc_mapper_for_tests() -> DefaultDocMapper {
         {
             "store_source": true,
             "default_search_fields": [
-                "body", "attributes.server", "attributes.server.status"
+                "body", "attributes.server", "attributes.server\\.status"
             ],
             "timestamp_field": "timestamp",
             "sort_by": {