PR #62 follow-up 21: reject repeated and output-colliding trace groupBy fields

docs/tool-api.md

@@ -244,8 +244,10 @@ traces dataset:
 `endTime`; both are resolved once and applied as exact, half-open bounds (start
 inclusive, end exclusive) on the dataset's time partition, so subminute ranges
 keep their full precision. `groupBy` takes up to 5
-exact field names. `filters` maps exact field names to JSON scalars: strings,
-numbers and booleans test equality, and `null` tests `IS NULL`. `bucketCount`
+exact field names. Each field may appear only once, and `time`, `span_count`
+and `trace_count` are rejected because they are output column names. `filters`
+maps exact field names to JSON scalars: strings, numbers and booleans test
+equality, and `null` tests `IS NULL`. `bucketCount`
 is 1–100, default 60. It approximates the number of `DATE_BIN` buckets over the
 dataset's time partition, or `p_timestamp` if none is configured. Buckets are
 whole seconds, sized from the exact range and aligned to `startTime`. The dataset

src/tool_executor.rs

@@ -1580,6 +1580,16 @@ fn trace_activity_group_by(arguments: &Value, fields: &[String]) -> Result<Vec<S
     if let Some(field) = group_by.iter().find(|field| !fields.contains(field)) {
         return Err(format!("groupBy field `{field}` does not exist"));
     }
+    for (index, field) in group_by.iter().enumerate() {
+        if group_by[..index].contains(field) {
+            return Err(format!("groupBy field `{field}` is listed more than once"));
+        }
+        if ["time", "span_count", "trace_count"].contains(&field.as_str()) {
+            return Err(format!(
+                "groupBy field `{field}` collides with the `{field}` output column"
+            ));
+        }
+    }
     Ok(group_by)
 }
 
@@ -2739,6 +2749,31 @@ mod tests {
         assert!(bounded_tool_sql(&query).is_ok());
     }
 
+    #[actix_web::test]
+    async fn trace_activity_rejects_repeated_and_output_colliding_group_by() {
+        let fields = ["service.name", "time", "span_count", "trace_count"].map(str::to_owned);
+        assert_eq!(
+            super::trace_activity_group_by(
+                &json!({ "groupBy": ["service.name", "service.name"] }),
+                &fields
+            ),
+            Err("groupBy field `service.name` is listed more than once".to_owned())
+        );
+        for field in ["time", "span_count", "trace_count"] {
+            assert_eq!(
+                super::trace_activity_group_by(&json!({ "groupBy": [field] }), &fields),
+                Err(format!(
+                    "groupBy field `{field}` collides with the `{field}` output column"
+                ))
+            );
+        }
+        assert_eq!(
+            super::trace_activity_group_by(&json!({ "groupBy": ["service.name"] }), &fields)
+                .unwrap(),
+            ["service.name".to_owned()]
+        );
+    }
+
     #[actix_web::test]
     async fn trace_activity_requires_span_columns_and_uses_the_time_partition() {
         let fields =