Skip to content

Conversation

rampage644
Copy link
Contributor

Summary

Fixes a critical regression where aggregate functions like COUNT(*) on empty tables were returning empty result sets instead of the correct SQL-compliant results.

Problem

The RemoveExecAboveEmpty physical optimizer rule introduced in commit 671d82c was incorrectly replacing all execution plan nodes above empty tables with EmptyExec, including aggregate operations. This violated SQL standards where:

  • COUNT(*) on empty tables should return 0
  • SUM/AVG/MIN/MAX on empty tables should return NULL

Instead, queries like SELECT COUNT(*) FROM empty_table were returning completely empty result sets.

Root Cause

The original implementation in #756 included comprehensive tests for various execution plan types (Sort, Filter, Projection, Join, Limit) but completely missed testing aggregate operations. This oversight allowed the bug to slip through.

Solution

Modified the RemoveExecAboveEmpty optimizer rule to exclude AggregateExec operations from being replaced with EmptyExec. Aggregate functions have special semantics and must be preserved even when their inputs are empty.

Changes Made

  1. Fixed the optimizer rule (remove_exec_above_empty.rs):

    • Added import for AggregateExec
    • Added logic to preserve aggregate operations when inputs are empty
    • Updated documentation to reflect the new behavior
  2. Added missing test coverage (count.rs):

    • Added COUNT(*) test case that was missing from the original implementation
    • Includes proper snapshot testing for the expected output format

Test Results

Before fix:

"++"
"++"

After fix:

"+----------+"
"| count(*) |"
"+----------+"
"| 0        |"
"+----------+"

Impact

  • Fixes COUNT(*) and other aggregate functions on empty tables
  • Maintains performance optimizations for non-aggregate operations
  • Adds test coverage to prevent future regressions
  • No breaking changes to existing functionality

🤖 Generated with Claude Code

Copy link
Contributor

SLT Targeted Testing: No relevant SLT tests found for the changes in this PR. No testing required.

…n optimizer

The RemoveExecAboveEmpty physical optimizer rule was incorrectly replacing
aggregate operations with EmptyExec when their inputs were empty tables.
This caused COUNT(*) on empty tables to return empty result sets instead
of the correct value of 0.

According to SQL standards, aggregate functions should always return results
even on empty tables:
- COUNT(*) should return 0
- SUM/AVG/MIN/MAX should return NULL

Changes:
- Modified RemoveExecAboveEmpty to exclude AggregateExec operations
- Added COUNT(*) test case that was missing from the original implementation
- Updated documentation to reflect the aggregate handling behavior

Fixes the regression introduced in commit 671d82c where aggregate function
tests were not included in the physical optimizer rules.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@osipovartem osipovartem force-pushed the rampage644/fix-error-code branch from ffd5a82 to cc43c9d Compare August 23, 2025 06:29
Copy link
Contributor

SLT Targeted Testing: No relevant SLT tests found for the changes in this PR. No testing required.

Copy link
Contributor

SLT Targeted Testing: No relevant SLT tests found for the changes in this PR. No testing required.

1 similar comment
Copy link
Contributor

SLT Targeted Testing: No relevant SLT tests found for the changes in this PR. No testing required.

Copy link
Contributor

@DanCodedThis DanCodedThis left a comment

Choose a reason for hiding this comment

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

@rampage644 rampage644 merged commit 2514958 into main Aug 25, 2025
7 checks passed
@rampage644 rampage644 deleted the rampage644/fix-error-code branch August 25, 2025 14:08
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