Skip to content

Remove per-row runtime_error from CastInputsToJson in aggregate functions#243

Merged
anasdorbani merged 2 commits intodevfrom
copilot/sub-pr-239
Mar 2, 2026
Merged

Remove per-row runtime_error from CastInputsToJson in aggregate functions#243
anasdorbani merged 2 commits intodevfrom
copilot/sub-pr-239

Conversation

Copy link
Contributor

Copilot AI commented Mar 2, 2026

CastInputsToJson threw a std::runtime_error when context_columns was absent, but this was called on every row during aggregation — meaning a bad prompt would produce one exception per row at execution time rather than failing once at bind time.

Changes

  • src/functions/aggregate/aggregate.cpp: Removed the else { throw std::runtime_error(...) } branch from CastInputsToJson. The context_columns presence check is already enforced at bind time by ValidatePromptStructFields, making the per-row throw unreachable dead code.
// Before
if (prompt_context_json.contains("context_columns")) {
    context_columns = prompt_context_json["context_columns"];
    prompt_context_json.erase("context_columns");
} else {
    throw std::runtime_error("Missing 'context_columns' in second argument...");
}

// After — bind-time validation in ValidatePromptStructFields is sufficient
if (prompt_context_json.contains("context_columns")) {
    context_columns = prompt_context_json["context_columns"];
    prompt_context_json.erase("context_columns");
}

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…ion is already done at bind time

Co-authored-by: anasdorbani <95044293+anasdorbani@users.noreply.github.com>
Copilot AI changed the title [WIP] Update implementation based on WASM support feedback Remove per-row runtime_error from CastInputsToJson in aggregate functions Mar 2, 2026
@anasdorbani anasdorbani marked this pull request as ready for review March 2, 2026 20:07
Copilot AI review requested due to automatic review settings March 2, 2026 20:07
@anasdorbani anasdorbani merged commit 0b7faba into dev Mar 2, 2026
1 check passed
@anasdorbani anasdorbani deleted the copilot/sub-pr-239 branch March 2, 2026 20:07
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts Flock’s aggregate-function input parsing to avoid throwing a runtime exception during aggregation when context_columns is absent, relying instead on bind-time validation.

Changes:

  • Removed the runtime std::runtime_error branch in AggregateFunctionBase::CastInputsToJson when context_columns is missing.
  • Leaves enforcement of context_columns presence to bind-time checks (ValidatePromptStructFields).

Comment on lines 128 to 133
auto prompt_context_json = CastVectorOfStructsToJson(inputs[1], count);
auto context_columns = nlohmann::json::array();
if (prompt_context_json.contains("context_columns")) {
context_columns = prompt_context_json["context_columns"];
prompt_context_json.erase("context_columns");
} else {
throw std::runtime_error("Missing 'context_columns' in second argument. The prompt struct must include context_columns.");
}
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValidatePromptStructFields only guarantees the prompt STRUCT type contains a context_columns field, but CastVectorOfStructsToJson currently omits the context_columns key when the list is empty (and also returns an empty object when count == 0). With this change, those cases will silently produce context_columns = [] instead of throwing, which can change aggregate behavior (often resulting in NULL output). Consider ensuring CastVectorOfStructsToJson always materializes context_columns as an empty array when present-but-empty, or add an explicit validation/error here if empty context is not supported.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot apply changes based on this feedback

Comment on lines 130 to 133
if (prompt_context_json.contains("context_columns")) {
context_columns = prompt_context_json["context_columns"];
prompt_context_json.erase("context_columns");
} else {
throw std::runtime_error("Missing 'context_columns' in second argument. The prompt struct must include context_columns.");
}
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change alters runtime behavior when context_columns is empty/missing in the JSON produced at execution time. Please add a unit/integration test that exercises an aggregate call with context_columns := [] (and/or an empty input chunk) to lock in the intended behavior (error vs NULL result).

Copilot uses AI. Check for mistakes.
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.

3 participants