Warn when AT ALL drops ungrouped WHERE filters

nicosuave · nicosuave · commit c29981c7ab13 · 2026-07-02T08:21:37.000-07:00
diff --git a/include/yardstick_ffi.h b/include/yardstick_ffi.h
@@ -313,6 +313,7 @@ struct YardstickAggregateResult {
     bool had_aggregate;
     char* expanded_sql;
     char* error;
+    char* warnings;
 };
 
 struct YardstickMeasureAggResult {
diff --git a/src/yardstick_extension.cpp b/src/yardstick_extension.cpp
@@ -6,6 +6,7 @@
 #include "duckdb/parser/statement/extension_statement.hpp"
 #include "duckdb/function/table_function.hpp"
 #include "duckdb/main/connection.hpp"
+#include "duckdb/logging/logger.hpp"
 
 // Include FFI types header
 #include "yardstick_ffi.h"
@@ -222,6 +223,8 @@ static std::string RewritePercentileWithinGroup(const std::string &sql) {
 // TABLE FUNCTION: yardstick(sql) - Execute SQL with AGGREGATE() expansion
 //=============================================================================
 
+static void HandleAggregateWarnings(ClientContext &context, const string &warnings);
+
 struct YardstickQueryData : public TableFunctionData {
     string original_sql;
     string rewritten_sql;
@@ -235,6 +238,9 @@ static unique_ptr<FunctionData> YardstickQueryBind(ClientContext &context,
                                                      vector<string> &names) {
     auto data = make_uniq<YardstickQueryData>();
     data->original_sql = input.inputs[0].GetValue<string>();
+    if (input.inputs.size() > 1) {
+        HandleAggregateWarnings(context, input.inputs[1].GetValue<string>());
+    }
 
     // Rewrite the SQL using our Rust code
     if (yardstick_has_aggregate(data->original_sql.c_str())) {
@@ -330,6 +336,49 @@ static bool StartsWithSemantic(const std::string &query, std::string &stripped_q
     return true;
 }
 
+static std::string EscapeSqlStringLiteral(const std::string &value) {
+    string escaped;
+    for (char c : value) {
+        if (c == '\'') {
+            escaped += "''";
+        } else {
+            escaped += c;
+        }
+    }
+    return escaped;
+}
+
+static std::string AggregateWarnings(YardstickAggregateResult &result) {
+    return result.warnings ? string(result.warnings) : string();
+}
+
+static bool WarningsAsErrors(ClientContext &context) {
+    Value setting;
+    if (!context.TryGetCurrentSetting("warnings_as_errors", setting) || setting.IsNull()) {
+        return false;
+    }
+    return setting.GetValue<bool>();
+}
+
+static void HandleAggregateWarnings(ClientContext &context, const string &warnings) {
+    if (warnings.empty()) {
+        return;
+    }
+    if (WarningsAsErrors(context)) {
+        throw InvalidInputException("%s", warnings);
+    }
+    DUCKDB_LOG_WARNING(context, "%s", warnings.c_str());
+}
+
+static std::string YardstickWrapperSql(const std::string &expanded_sql, const std::string &warnings) {
+    string wrapper_sql = "SELECT * FROM yardstick('" + EscapeSqlStringLiteral(expanded_sql) + "'";
+    if (!warnings.empty()) {
+        wrapper_sql += ", '" + EscapeSqlStringLiteral(warnings) + "'";
+    }
+    wrapper_sql += ")";
+    return wrapper_sql;
+}
+
 #if YARDSTICK_TOKEN_PARSE_FN
 // DuckDB main: parse_function receives the post-PEG-failure token tail rather
 // than the query string. Yardstick rewrites queries earlier, in
@@ -367,20 +416,10 @@ ParserExtensionParseResult yardstick_parse(ParserExtensionInfo *,
 
         if (result.had_aggregate) {
             string expanded_sql(result.expanded_sql);
+            string warnings = AggregateWarnings(result);
             yardstick_free_aggregate_result(result);
 
-            // Escape single quotes for embedding in string literal
-            string escaped_sql;
-            for (char c : expanded_sql) {
-                if (c == '\'') {
-                    escaped_sql += "''";
-                } else {
-                    escaped_sql += c;
-                }
-            }
-
-            // Wrap in table function call
-            string wrapper_sql = "SELECT * FROM yardstick('" + escaped_sql + "')";
+            string wrapper_sql = YardstickWrapperSql(expanded_sql, warnings);
 
             Parser parser;
             parser.ParseQuery(wrapper_sql);
@@ -469,6 +508,7 @@ ParserOverrideResult yardstick_parser_override(ParserExtensionInfo *,
 
         if (result.had_aggregate) {
             string expanded_sql(result.expanded_sql);
+            string warnings = AggregateWarnings(result);
             yardstick_free_aggregate_result(result);
 
             // Validate the expanded SQL parses. If expansion produced garbage
@@ -489,16 +529,7 @@ ParserOverrideResult yardstick_parser_override(ParserExtensionInfo *,
                              validation_parser.statements[0]->type == StatementType::SELECT_STATEMENT;
 
             if (is_select) {
-                string escaped_sql;
-                for (char c : expanded_sql) {
-                    if (c == '\'') {
-                        escaped_sql += "''";
-                    } else {
-                        escaped_sql += c;
-                    }
-                }
-
-                string wrapper_sql = "SELECT * FROM yardstick('" + escaped_sql + "')";
+                string wrapper_sql = YardstickWrapperSql(expanded_sql, warnings);
                 Parser parser;
                 parser.ParseQuery(wrapper_sql);
                 return ParserOverrideResult(std::move(parser.statements));
@@ -594,20 +625,11 @@ BoundStatement yardstick_bind(ClientContext &context, Binder &binder,
 
             if (result.had_aggregate) {
                 string expanded_sql(result.expanded_sql);
+                string warnings = AggregateWarnings(result);
                 yardstick_free_aggregate_result(result);
 
-                // Escape single quotes for embedding in string literal
-                string escaped_sql;
-                for (char c : expanded_sql) {
-                    if (c == '\'') {
-                        escaped_sql += "''";
-                    } else {
-                        escaped_sql += c;
-                    }
-                }
-
                 // Rebind through table function so rewritten SQL executes with normal planning
-                string wrapper_sql = "SELECT * FROM yardstick('" + escaped_sql + "')";
+                string wrapper_sql = YardstickWrapperSql(expanded_sql, warnings);
                 Parser parser;
                 parser.ParseQuery(wrapper_sql);
                 auto statements = std::move(parser.statements);
@@ -678,6 +700,9 @@ static void LoadInternal(ExtensionLoader &loader) {
     TableFunction query_func("yardstick", {LogicalType::VARCHAR},
                              YardstickQueryFunction, YardstickQueryBind);
     loader.RegisterFunction(query_func);
+    TableFunction query_func_with_warnings("yardstick", {LogicalType::VARCHAR, LogicalType::VARCHAR},
+                                           YardstickQueryFunction, YardstickQueryBind);
+    loader.RegisterFunction(query_func_with_warnings);
 }
 
 void YardstickExtension::Load(ExtensionLoader &loader) {
diff --git a/test/sql/measures.test b/test/sql/measures.test
@@ -114,6 +114,23 @@ FROM sales_v;
 2023	EU	225.0
 2023	US	225.0
 
+# AT (ALL dim) drops outer WHERE filters on dimensions that are not grouped.
+# Surface a warning through DuckDB; warnings_as_errors makes this test assert it is emitted.
+statement ok
+SET enable_logging = true;
+
+statement ok
+SET warnings_as_errors = true;
+
+statement error
+SEMANTIC SELECT region, 100.0 * AGGREGATE(revenue) / AGGREGATE(revenue) AT (ALL region) AS pct_of_total
+FROM sales_v
+WHERE year = 2023;
+----
+
+statement ok
+SET warnings_as_errors = false;
+
 # ORDER BY expression referencing a named aggregate that expands to a subquery (#28)
 query IIRR
 SEMANTIC SELECT
diff --git a/yardstick-rs/src/ffi.rs b/yardstick-rs/src/ffi.rs
@@ -13,8 +13,7 @@ use std::ptr;
 use crate::sql::{
     drop_measure_view_from_sql, expand_aggregate_with_at, expand_curly_braces,
     get_measure_aggregation, has_aggregate_function, has_as_measure, has_at_syntax,
-    has_curly_brace_measure, process_create_view,
-    has_implicit_measure_refs, has_measure_at_refs,
+    has_curly_brace_measure, has_implicit_measure_refs, has_measure_at_refs, process_create_view,
 };
 
 /// Result from processing CREATE VIEW with AS MEASURE
@@ -39,6 +38,8 @@ pub struct YardstickAggregateResult {
     pub expanded_sql: *mut c_char,
     /// Error message (null if success)
     pub error: *mut c_char,
+    /// Warning messages separated by newlines (null if none)
+    pub warnings: *mut c_char,
 }
 
 /// Check if SQL contains "AS MEASURE" pattern
@@ -179,6 +180,7 @@ pub extern "C" fn yardstick_expand_aggregate(sql: *const c_char) -> YardstickAgg
             had_aggregate: false,
             expanded_sql: ptr::null_mut(),
             error: to_c_string("Error: null sql pointer"),
+            warnings: ptr::null_mut(),
         };
     }
 
@@ -190,6 +192,7 @@ pub extern "C" fn yardstick_expand_aggregate(sql: *const c_char) -> YardstickAgg
                     had_aggregate: false,
                     expanded_sql: ptr::null_mut(),
                     error: to_c_string(&format!("Error: invalid UTF-8: {e}")),
+                    warnings: ptr::null_mut(),
                 };
             }
         }
@@ -204,6 +207,11 @@ pub extern "C" fn yardstick_expand_aggregate(sql: *const c_char) -> YardstickAgg
             .error
             .map(|s| to_c_string(&s))
             .unwrap_or(ptr::null_mut()),
+        warnings: if result.warnings.is_empty() {
+            ptr::null_mut()
+        } else {
+            to_c_string(&result.warnings.join("\n"))
+        },
     }
 }
 
@@ -299,6 +307,7 @@ pub extern "C" fn yardstick_free_create_view_result(result: YardstickCreateViewR
 pub extern "C" fn yardstick_free_aggregate_result(result: YardstickAggregateResult) {
     yardstick_free(result.expanded_sql);
     yardstick_free(result.error);
+    yardstick_free(result.warnings);
 }
 
 // Helper: convert Rust string to C string
diff --git a/yardstick-rs/src/sql/measures.rs b/yardstick-rs/src/sql/measures.rs
