aggregate_records should default fields to *#3529
Open
souvikghosh04 wants to merge 2 commits intomainfrom
Open
aggregate_records should default fields to *#3529souvikghosh04 wants to merge 2 commits intomainfrom
souvikghosh04 wants to merge 2 commits intomainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the aggregate_records built-in MCP tool to make field optional for the common count(*) scenario by declaring a JSON Schema default ("*"), removing field from the required list, and adding runtime validation to only apply that default for function=count.
Changes:
- Updated
aggregate_recordsinput schema:fieldis no longer required and declares"default": "*". - Updated argument parsing to default
fieldto"*"only whenfunction=count; non-count functions now error iffieldis omitted. - Updated unit tests to validate the schema’s required list and
fielddefault, and to adjust missing-field validation cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs | Makes field optional in the schema and implements defaulting/validation behavior in argument parsing. |
| src/Service.Tests/Mcp/AggregateRecordsToolTests.cs | Updates schema assertions and missing-argument test cases to reflect the new optional field behavior. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why make this change?
aggregate_recordsshould default fields to*#3280aggregate_recordsMCP tool requires callers to explicitly pass afieldargument, even for the most common case (function: count) where*is the only sensible value. The issue requests defaultingfieldto"*"in the JSON Schema so LLM clients (and other MCP callers) can omit the argument forcount-all scenarios, aligning the tool with the conventionalCOUNT(*)shape.What is this change?
aggregate_recordstool's input schema inAggregateRecordsTool.cs:"default": "*"to thefieldproperty and clarified its description."field"from the schema'srequiredarray (now onlyentityandfunctionare required).TryParseAndValidateArgumentsso that whenfieldis omitted:function: count⇒fielddefaults to"*"(existingcount(*)validation/path is preserved).function: avg | sum | min | max⇒ returns anInvalidArgumentserror explaining a specific numeric field is required, since"*"is invalid for those aggregations.fieldis explicitly supplied (including thefield='*'+ non-count rejection and thedistinct+count(*)rejection).How was this tested?
GetToolMetadata_HasRequiredSchemaPropertiesto assert:fieldis no longer in therequiredarray.fieldproperty declares"default": "*".AggregateRecords_MissingOrInvalidRequiredArgs_ReturnsInvalidArgumentsdata rows to:avg/sumwithoutfieldreturn anInvalidArgumentserror mentioningfield.AggregateRecordsToolTestspass locally.Sample Request(s)
Before this change (required):
{ "tool": "aggregate_records", "arguments": { "entity": "Book", "function": "count", "field": "*" } }After this change (
fieldmay be omitted forcount):{ "tool": "aggregate_records", "arguments": { "entity": "Book", "function": "count" } }Non-count functions still require an explicit field: