From 2614c975d90f289d1ca1ba1c291d6118b45e44a1 Mon Sep 17 00:00:00 2001
From: Pascal Seitz <pascal.seitz@gmail.com>
Date: Wed, 24 Jan 2024 11:09:36 +0800
Subject: [PATCH] fix agg result on empty index

empty indices return empty intermediate aggregation results. To get the
correct result structure we need to call `into_final` instead of early exit.

fixes #4437
---
 quickwit/quickwit-search/src/root.rs          | 33 ++++++++++++-------
 .../aggregations/0001-aggregations.yaml       | 18 ++++++++++
 .../aggregations/_setup.quickwit.yaml         | 24 ++++++++++++++
 .../aggregations/_teardown.quickwit.yaml      |  4 +++
 4 files changed, 68 insertions(+), 11 deletions(-)

diff --git a/quickwit/quickwit-search/src/root.rs b/quickwit/quickwit-search/src/root.rs
index 4b187033862..3ef7c75a40f 100644
--- a/quickwit/quickwit-search/src/root.rs
+++ b/quickwit/quickwit-search/src/root.rs
@@ -895,25 +895,39 @@ async fn root_search_aux(
 }
 
 fn finalize_aggregation(
-    intermediate_aggregation_result_bytes: &[u8],
+    intermediate_aggregation_result_bytes_opt: Option<Vec<u8>>,
     aggregations: QuickwitAggregations,
     searcher_context: &SearcherContext,
-) -> crate::Result<String> {
+) -> crate::Result<Option<String>> {
     let merge_aggregation_result = match aggregations {
         QuickwitAggregations::FindTraceIdsAggregation(_) => {
+            let Some(intermediate_aggregation_result_bytes) =
+                intermediate_aggregation_result_bytes_opt
+            else {
+                return Ok(None);
+            };
             // The merge collector has already merged the intermediate results.
-            let aggs: Vec<Span> = postcard::from_bytes(intermediate_aggregation_result_bytes)?;
+            let aggs: Vec<Span> = postcard::from_bytes(&intermediate_aggregation_result_bytes)?;
             serde_json::to_string(&aggs)?
         }
         QuickwitAggregations::TantivyAggregations(aggregations) => {
-            let intermediate_aggregation_results: IntermediateAggregationResults =
-                postcard::from_bytes(intermediate_aggregation_result_bytes)?;
+            let intermediate_aggregation_results =
+                if let Some(intermediate_aggregation_result_bytes) =
+                    intermediate_aggregation_result_bytes_opt
+                {
+                    let intermediate_aggregation_results: IntermediateAggregationResults =
+                        postcard::from_bytes(&intermediate_aggregation_result_bytes)?;
+                    intermediate_aggregation_results
+                } else {
+                    // Default, to return correct structure
+                    Default::default()
+                };
             let final_aggregation_results: AggregationResults = intermediate_aggregation_results
                 .into_final_result(aggregations, &searcher_context.get_aggregation_limits())?;
             serde_json::to_string(&final_aggregation_results)?
         }
     };
-    Ok(merge_aggregation_result)
+    Ok(Some(merge_aggregation_result))
 }
 
 fn finalize_aggregation_if_any(
@@ -925,15 +939,12 @@ fn finalize_aggregation_if_any(
         return Ok(None);
     };
     let aggregations: QuickwitAggregations = serde_json::from_str(aggregations_json)?;
-    let Some(intermediate_result_bytes) = intermediate_aggregation_result_bytes_opt else {
-        return Ok(None);
-    };
     let aggregation_result_json = finalize_aggregation(
-        &intermediate_result_bytes[..],
+        intermediate_aggregation_result_bytes_opt,
         aggregations,
         searcher_context,
     )?;
-    Ok(Some(aggregation_result_json))
+    Ok(aggregation_result_json)
 }
 
 /// Checks that all of the index researched as found.
diff --git a/quickwit/rest-api-tests/scenarii/aggregations/0001-aggregations.yaml b/quickwit/rest-api-tests/scenarii/aggregations/0001-aggregations.yaml
index deb1a0fb0b1..064f23dfd5d 100644
--- a/quickwit/rest-api-tests/scenarii/aggregations/0001-aggregations.yaml
+++ b/quickwit/rest-api-tests/scenarii/aggregations/0001-aggregations.yaml
@@ -192,3 +192,21 @@ expected:
       - doc_count: 3
         key: 100.0
 
+--- 
+# Test histogram empty result on empty index
+method: [GET]
+engines:
+  - quickwit
+endpoint: _elastic/empty_aggregations/_search
+json:
+  query: { match_all: {} }
+  aggs:
+    metrics:
+      histogram:
+        field: response
+        interval: 50
+expected:
+  aggregations:
+    metrics:
+      buckets: []
+
diff --git a/quickwit/rest-api-tests/scenarii/aggregations/_setup.quickwit.yaml b/quickwit/rest-api-tests/scenarii/aggregations/_setup.quickwit.yaml
index 8c415fe08eb..37ce5fcd0a6 100644
--- a/quickwit/rest-api-tests/scenarii/aggregations/_setup.quickwit.yaml
+++ b/quickwit/rest-api-tests/scenarii/aggregations/_setup.quickwit.yaml
@@ -3,6 +3,10 @@ method: DELETE
 endpoint: indexes/aggregations
 status_code: null
 ---
+method: DELETE
+endpoint: indexes/empty_aggregations
+status_code: null
+---
 # Create index
 method: POST
 endpoint: indexes/
@@ -23,6 +27,26 @@ json:
         fast: true
 sleep_after: 3
 ---
+# Create empty index
+method: POST
+endpoint: indexes/
+json:
+  version: "0.7"
+  index_id: empty_aggregations
+  doc_mapping:
+    mode: dynamic
+    dynamic_mapping:
+      tokenizer: default
+      fast: true
+    field_mappings:
+      - name: date
+        type: datetime
+        input_formats:
+          - rfc3339
+        fast_precision: seconds
+        fast: true
+sleep_after: 3
+---
 # Ingest documents
 method: POST
 endpoint: aggregations/ingest
diff --git a/quickwit/rest-api-tests/scenarii/aggregations/_teardown.quickwit.yaml b/quickwit/rest-api-tests/scenarii/aggregations/_teardown.quickwit.yaml
index bcc4721d8c1..03ff736e7d2 100644
--- a/quickwit/rest-api-tests/scenarii/aggregations/_teardown.quickwit.yaml
+++ b/quickwit/rest-api-tests/scenarii/aggregations/_teardown.quickwit.yaml
@@ -1,3 +1,7 @@
 # # Delete index
 method: DELETE
 endpoint: indexes/aggregations
+---
+method: DELETE
+endpoint: indexes/empty_aggregations
+