Skip to content

rationalize aggregate operator error value handling#6710

Merged
nwt merged 1 commit intomainfrom
aggregate-error-handling
Mar 11, 2026
Merged

rationalize aggregate operator error value handling#6710
nwt merged 1 commit intomainfrom
aggregate-error-handling

Conversation

@nwt
Copy link
Copy Markdown
Member

@nwt nwt commented Mar 11, 2026

Error value handling in the aggregate operator differs between sam and vam, and neither implementation is really consistent with the philosophy for error value handling in other parts of the system. Improve the situation by making both implementations consistent about ignoring error("quiet") and preserving other errors.

Partially fixes #5039

Error value handling in the aggregate operator differs between sam and
vam, and neither implementation is really consistent with the philosophy
for error value handling in other parts of the system.  Improve the
situation by making both implementations consistent about ignoring
error("quiet") and preserving other errors.
@nwt nwt requested a review from a team March 11, 2026 04:35
output: |
count
10
{count:12}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the correct result be 11 since we ignore error("quiet") or am I missing something?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Or is error("quiet") only ignored when its a key?

Copy link
Copy Markdown
Collaborator

@mattnibs mattnibs Mar 11, 2026

Choose a reason for hiding this comment

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

If the above is true its seems inconsistent with the below results of feeding error("quiet") into fuse or dcount.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Excellent point! I meant to add a comment about this so thanks for catching it.

I feel like you're probably right that bare count() should ignore error("quiet") but I'm not really sure.

If you look at SQL, select count(*) from (select null) feels analogous, and that returns 1, but of course error("quiet") isn't the same as null.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's just ship this and see how it feels.

@nwt nwt merged commit 3de5ba8 into main Mar 11, 2026
2 checks passed
@nwt nwt deleted the aggregate-error-handling branch March 11, 2026 18:36
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.

collect() and union() skipping null values

2 participants