Skip to content

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

Merged
VibeSWE merged 4 commits into
mainfrom
fix/pr62-21-trace-groupby-collisions
Oct 24, 2026
Merged

VibeSWE merged 4 commits into
mainfrom
fix/pr62-21-trace-groupby-collisions

Conversation

@VibeSWE

@VibeSWE VibeSWE commented Oct 24, 2026 •

Copy link
Copy Markdown

Problem

get_trace_activity projects each groupBy field next to the fixed output aliases time, span_count and trace_count. trace_activity_group_by only checked that each field exists. A repeated entry, or a trace field literally named like an output alias, therefore produced duplicate projection names, and DataFusion rejected the plan with an error that never mentions groupBy. Reproduced against the DataFusion trace fixture:

  • groupBy: ["service.name", "service.name"] gives Plan("Projections require unique expression names but the expression \"traces.service.name\" at position 1 and \"traces.service.name\" at position 2 have the same name. ...")
  • groupBy: ["time"] (also span_count and trace_count) on a dataset that has such a column gives Schema error: Schema contains qualified field name traces.time and unqualified field name time which would be ambiguous

Fix

After the existence check, trace_activity_group_by now rejects:

  • a field listed more than once: groupBy field `x` is listed more than once
  • a field named time, span_count or trace_count: groupBy field `x` collides with the `x` output column

The query shape and output rows don't change. docs/tool-api.md documents the rule.

Tests

  • New trace_activity_rejects_repeated_and_output_colliding_group_by asserts the exact error for the repeated case and for each of the three aliases, and checks that a normal groupBy still passes. Without the fix, all four inputs return Ok, so the test fails.
  • cargo test --lib tool_: 47 passed. cargo clippy -- -D warnings: clean. cargo clippy --all-targets: no warnings in touched files.

Upstream fidelity

This adds about 10 lines of validation to upstream-owned code (port of parseablehq/parseable 32979cb, parseablehq#1831), with no refactor. An upstream issue/PR draft is prepared separately; nothing has been filed upstream.

Stacking

Stacked on #62 (base upstream/tool-catalog-1831). Retarget to main after #62 merges.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

VibeSWE and others added 3 commits October 24, 2026 16:00
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>
… fix/pr62-21-trace-groupby-collisions

# Conflicts:
#	src/tool_executor.rs
@VibeSWE
VibeSWE changed the base branch from upstream/tool-catalog-1831 to main October 24, 2026 23:25
…upby-collisions

# Conflicts:
#	docs/tool-api.md
@VibeSWE
VibeSWE merged commit 4e0ca0b into main Oct 24, 2026
9 checks passed
@VibeSWE
VibeSWE deleted the fix/pr62-21-trace-groupby-collisions branch October 24, 2026 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant