From e01cfae35918bb7266d6e477ed8874b9c498dec7 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Tue, 19 May 2026 17:01:08 -0500 Subject: [PATCH] perf+docs: fix IsValid allocation regression and document FilterGroup asymmetry Code review follow-up on 6f85358: - CalculationFilterGroup.IsValid() previously called GetValidFilters().Any(), which materialized a full filtered List just to check existence. Reimplement directly as Filters.Any(x => x.IsValid()) so IsValid() recovers its pre-refactor O(1) early-exit behavior without allocations. GetValidFilters() remains for callers that need the full materialized list. - QueryConfigExtensions.GetAllColumnIds line 16 uses FilterGroup (not CalculationFilterGroup), which pre-filters in its JsonConstructor and has no GetValidFilters() method. Add a comment to document the intentional asymmetry between line 14 and line 16. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../ExpressionFactory/Query/CalculationFilterGroup.cs | 2 +- .../ExpressionFactory/Query/QueryConfigExtensions.cs | 1 + .../ExpressionFactory/Query/CalculationFilterGroup.cs | 2 +- .../ExpressionFactory/Query/QueryConfigExtensions.cs | 1 + 4 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs b/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs index 4580779..ce0f011 100644 --- a/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs +++ b/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs @@ -29,6 +29,6 @@ public class CalculationFilterGroup public bool IsValid() { - return GetValidFilters().Any(); + return Filters != null && Filters.Any(x => x.IsValid()); } } diff --git a/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs b/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs index 88f72c2..7d62951 100644 --- a/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs +++ b/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs @@ -13,6 +13,7 @@ public static class QueryConfigExtensions return queryConfig.Values.SelectMany(value => value.CalculationDataColumnIds) .Union(queryConfig.Values.SelectMany(x => x.FilterGroups.SelectMany(y => y.GetValidFilters().Select(f => f.DataColumnId)))) .Union(queryConfig.Rows.Select(row => row.DataColumnId)) + // FilterGroup.Filters is pre-filtered at construction (see FilterGroup.cs JsonConstructor); no GetValidFilters() equivalent is needed here. .Union(queryConfig.FilterGroups.SelectMany(filterGroup => filterGroup.Filters.Select(filter => filter.DataColumnId))) .ToArray(); } diff --git a/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs b/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs index 5fb4e0d..ce4f1b1 100644 --- a/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs +++ b/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs @@ -29,6 +29,6 @@ public class CalculationFilterGroup public bool IsValid() { - return GetValidFilters().Any(); + return Filters != null && Filters.Any(x => x.IsValid()); } } diff --git a/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs b/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs index 692f4a5..33f623d 100644 --- a/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs +++ b/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs @@ -13,6 +13,7 @@ public static class QueryConfigExtensions return queryConfig.Values.SelectMany(value => value.CalculationDataColumnIds) .Union(queryConfig.Values.SelectMany(x => x.FilterGroups.SelectMany(y => y.GetValidFilters().Select(f => f.DataColumnId)))) .Union(queryConfig.Rows.Select(row => row.DataColumnId)) + // FilterGroup.Filters is pre-filtered at construction (see FilterGroup.cs JsonConstructor); no GetValidFilters() equivalent is needed here. .Union(queryConfig.FilterGroups.SelectMany(filterGroup => filterGroup.Filters.Select(filter => filter.DataColumnId))) .ToArray(); }