From fe17bf0e574ea0156e776c224bf01b8263c27e30 Mon Sep 17 00:00:00 2001
From: VibeSWE <338779468+VibeSWE@users.noreply.github.com>
Date: Sat, 24 Oct 2026 16:00:37 -0500
Subject: [PATCH] Reject repeated and output-colliding get_trace_activity
 groupBy fields

A repeated groupBy entry, or a field named time, span_count or
trace_count, produced duplicate projection names that DataFusion
rejected with an opaque plan/schema error. Validate these up front
with clear messages and document the rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 docs/tool-api.md     |  6 ++++--
 src/tool_executor.rs | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/docs/tool-api.md b/docs/tool-api.md
index fc9cca74f..18c541713 100644
--- a/docs/tool-api.md
+++ b/docs/tool-api.md
@@ -204,8 +204,10 @@ traces dataset:
 ```
 
 `dataset`, `startTime` and `endTime` are required. `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. The dataset
 must have traces telemetry and contain `span_span_id` and `span_trace_id`.
diff --git a/src/tool_executor.rs b/src/tool_executor.rs
index f2c477041..971ed1365 100644
--- a/src/tool_executor.rs
+++ b/src/tool_executor.rs
@@ -1457,6 +1457,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)
 }
 
@@ -2219,6 +2229,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_bucket_interval_is_bounded() {
         assert_eq!(