Skip to content

Commit d6b50d4

Browse files
dfa1claude
dfa1
and
claude
committed
refactor(compute): reduce Sonar complexity/param findings in fused kernels
Triaged the surviving compute-package findings (the deleted Mask/kernel files carried most of them): - FusedFilterAggregate.foldAggregate (S3776, was cognitive complexity 64): branch-split the 3-way domain fold into foldLongDomain / foldDoubleDomain / foldGeneric loop methods dispatched once, matching the sibling FusedFilterSum structure and the hot-loop branch-split rule. Semantics preserved exactly. - FusedFilterSum.longFilter{Long,Double}Agg (S107, 8 params): derive the XOR flip from `unsigned` at the method top instead of threading it as a separate param — computed once, zero hot-path cost. - PrimitiveFilter (S6878): use record patterns in the predicate-lowering switches instead of pattern-match variables. Skipped Predicate S1192 ("value" x6): the only fix is a public interface constant, which leaks public API for no real gain. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent cf0afc8 commit d6b50d4

3 files changed

Lines changed: 170 additions & 120 deletions

File tree

‎reader/src/main/java/io/github/dfa1/vortex/reader/compute/FusedFilterAggregate.java‎

Lines changed: 134 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,10 @@ static FilteredAggregate aggregate(Chunk chunk, RowFilter filter, String aggColu
7676
return foldAggregate(chunk.column(aggColumn), leaves, n);
7777
}
7878

79-
/// Runs the row loop folding `agg` over the rows every leaf accepts, with the aggregate domain
80-
/// (long / double / generic) and its accessors hoisted out of the loop so the inner body is a bare
81-
/// typed read plus a primitive accumulate.
79+
/// Dispatches the fold on the aggregate's value domain — a boxing-free long or double lane, else the
80+
/// generic boxing lane — so each specialized loop's inner body is a bare typed read plus a primitive
81+
/// accumulate, the hot-loop rule's branch-split idiom (hoist the domain choice once, gate one
82+
/// specialized loop per domain rather than branching per row).
8283
///
8384
/// @param agg the aggregate column (possibly a [io.github.dfa1.vortex.reader.array.MaskedArray])
8485
/// @param leaves the lowered filter leaves, all of which must accept a row for it to be selected
@@ -87,108 +88,158 @@ static FilteredAggregate aggregate(Chunk chunk, RowFilter filter, String aggColu
8788
private static FilteredAggregate foldAggregate(Array agg, RowPredicate[] leaves, long n) {
8889
Array aggData = NumericColumns.unwrap(agg);
8990
BoolArray aggValidity = NumericColumns.validityOf(agg);
90-
DType aggDtype = agg.dtype();
91-
boolean aggDoubleDomain = aggData instanceof DoubleArray || aggData instanceof FloatArray;
92-
boolean aggFloat = aggData instanceof FloatArray;
93-
boolean aggLongDomain = NumericColumns.isLongDomain(aggData);
94-
boolean aggUnsigned = aggLongDomain && aggData.dtype().isUnsigned();
91+
if (NumericColumns.isLongDomain(aggData)) {
92+
return foldLongDomain(aggData, aggValidity, aggData.dtype().isUnsigned(), leaves, n);
93+
}
94+
if (aggData instanceof DoubleArray || aggData instanceof FloatArray) {
95+
return foldDoubleDomain(aggData, aggValidity, aggData instanceof FloatArray, leaves, n);
96+
}
9597
// A numeric primitive without a specialized long/double lane (an f16 column) still has an
96-
// answerable SUM: it folds a boxing doubleValue() total into a Double, mirroring
97-
// NumericColumns.sumGeneric. A non-numeric column (Utf8/Binary/Bool/…) has no sum and reports null.
98-
boolean aggNumericPrimitive = !aggLongDomain && !aggDoubleDomain && aggDtype instanceof DType.Primitive;
98+
// answerable SUM: it folds a boxing doubleValue() total into a Double. A non-numeric column
99+
// (Utf8/Binary/Bool/…) has no sum and reports null.
100+
DType aggDtype = agg.dtype();
101+
return foldGeneric(agg, aggValidity, aggDtype, aggDtype instanceof DType.Primitive, leaves, n);
102+
}
99103

104+
/// The long-domain lane: reads each selected non-null row through [NumericColumns#widenLong(Array,
105+
/// boolean, long)] and folds a wrapping [Long] `SUM` plus an unsigned-aware `MIN` / `MAX`.
106+
///
107+
/// @param aggData the unwrapped long-domain aggregate array
108+
/// @param validity the aggregate validity bitmap, or `null` when every row is valid
109+
/// @param unsigned whether the column is unsigned (its order and widening zero-extend)
110+
/// @param leaves the lowered filter leaves, all of which must accept a row for it to be selected
111+
/// @param n the chunk's row count
112+
/// @return the fold: selected count, non-null count, the [Long] sum, and the [Long] `MIN` / `MAX`
113+
/// (`null` when no selected row is non-null)
114+
private static FilteredAggregate foldLongDomain(Array aggData, BoolArray validity, boolean unsigned,
115+
RowPredicate[] leaves, long n) {
100116
long selected = 0L;
101117
long nonNull = 0L;
102-
long longSum = 0L;
103-
double doubleSum = 0.0;
118+
long sum = 0L;
104119
boolean found = false;
105-
long minLong = 0L;
106-
long maxLong = 0L;
107-
double minDouble = 0.0;
108-
double maxDouble = 0.0;
109-
Object minObject = null;
110-
Object maxObject = null;
111-
120+
long min = 0L;
121+
long max = 0L;
112122
for (long i = 0; i < n; i++) {
113123
if (!allMatch(leaves, i)) {
114124
continue;
115125
}
116126
selected++;
117-
if (aggValidity != null && !aggValidity.getBoolean(i)) {
127+
if (validity != null && !validity.getBoolean(i)) {
118128
continue;
119129
}
120130
nonNull++;
121-
if (aggLongDomain) {
122-
long v = NumericColumns.widenLong(aggData, aggUnsigned, i);
123-
longSum += v;
124-
if (!found) {
125-
minLong = v;
126-
maxLong = v;
127-
found = true;
128-
} else {
129-
if (lessLong(v, minLong, aggUnsigned)) {
130-
minLong = v;
131-
}
132-
if (lessLong(maxLong, v, aggUnsigned)) {
133-
maxLong = v;
134-
}
131+
long v = NumericColumns.widenLong(aggData, unsigned, i);
132+
sum += v;
133+
if (!found) {
134+
min = v;
135+
max = v;
136+
found = true;
137+
} else {
138+
if (lessLong(v, min, unsigned)) {
139+
min = v;
135140
}
136-
} else if (aggDoubleDomain) {
137-
double v = aggFloat ? ((FloatArray) aggData).getFloat(i) : ((DoubleArray) aggData).getDouble(i);
138-
doubleSum += v;
139-
if (!found) {
140-
minDouble = v;
141-
maxDouble = v;
142-
found = true;
143-
} else {
144-
if (Double.compare(v, minDouble) < 0) {
145-
minDouble = v;
146-
}
147-
if (Double.compare(v, maxDouble) > 0) {
148-
maxDouble = v;
149-
}
141+
if (lessLong(max, v, unsigned)) {
142+
max = v;
150143
}
144+
}
145+
}
146+
return new FilteredAggregate(selected, nonNull, Long.valueOf(sum),
147+
found ? Long.valueOf(min) : null, found ? Long.valueOf(max) : null);
148+
}
149+
150+
/// The double-domain lane: reads each selected non-null row through the hoisted `f32`/`f64` accessor
151+
/// and folds a [Double] `SUM` plus a [Double#compare(double, double)]-ordered `MIN` / `MAX`.
152+
///
153+
/// @param aggData the unwrapped double-domain aggregate array
154+
/// @param validity the aggregate validity bitmap, or `null` when every row is valid
155+
/// @param isFloat whether the column is an `f32` ([FloatArray]) rather than an `f64`
156+
/// @param leaves the lowered filter leaves, all of which must accept a row for it to be selected
157+
/// @param n the chunk's row count
158+
/// @return the fold: selected count, non-null count, the [Double] sum, and the boxed `MIN` / `MAX`
159+
/// ([Float] for an `f32` column, `null` when no selected row is non-null)
160+
private static FilteredAggregate foldDoubleDomain(Array aggData, BoolArray validity, boolean isFloat,
161+
RowPredicate[] leaves, long n) {
162+
long selected = 0L;
163+
long nonNull = 0L;
164+
double sum = 0.0;
165+
boolean found = false;
166+
double min = 0.0;
167+
double max = 0.0;
168+
for (long i = 0; i < n; i++) {
169+
if (!allMatch(leaves, i)) {
170+
continue;
171+
}
172+
selected++;
173+
if (validity != null && !validity.getBoolean(i)) {
174+
continue;
175+
}
176+
nonNull++;
177+
double v = isFloat ? ((FloatArray) aggData).getFloat(i) : ((DoubleArray) aggData).getDouble(i);
178+
sum += v;
179+
if (!found) {
180+
min = v;
181+
max = v;
182+
found = true;
151183
} else {
152-
Object v = Values.valueAt(agg, i);
153-
if (aggNumericPrimitive) {
154-
doubleSum += ((Number) v).doubleValue();
184+
if (Double.compare(v, min) < 0) {
185+
min = v;
155186
}
156-
if (minObject == null) {
157-
minObject = v;
158-
maxObject = v;
159-
} else {
160-
if (Compare.values(v, minObject, aggDtype) < 0) {
161-
minObject = v;
162-
}
163-
if (Compare.values(v, maxObject, aggDtype) > 0) {
164-
maxObject = v;
165-
}
187+
if (Double.compare(v, max) > 0) {
188+
max = v;
166189
}
167190
}
168191
}
192+
return new FilteredAggregate(selected, nonNull, Double.valueOf(sum),
193+
boxDouble(found, min, isFloat), boxDouble(found, max, isFloat));
194+
}
169195

170-
// Explicit branches, not a ternary: a `Long : Double` conditional would binary-numeric promote
171-
// both arms to double and re-box the integer sum to Double, losing the long result type.
172-
Number sum;
173-
Object min;
174-
Object max;
175-
if (aggLongDomain) {
176-
sum = Long.valueOf(longSum);
177-
min = found ? Long.valueOf(minLong) : null;
178-
max = found ? Long.valueOf(maxLong) : null;
179-
} else if (aggDoubleDomain) {
180-
sum = Double.valueOf(doubleSum);
181-
min = boxDouble(found, minDouble, aggFloat);
182-
max = boxDouble(found, maxDouble, aggFloat);
183-
} else {
184-
// f16 (and any future numeric primitive without a specialized lane) sums as a Double via
185-
// the boxing doubleValue() fold, the additive identity (0.0) over zero non-null rows;
186-
// a genuinely non-numeric column (Utf8/Binary/Bool/…) has no answerable sum.
187-
sum = aggNumericPrimitive ? Double.valueOf(doubleSum) : null;
188-
min = minObject;
189-
max = maxObject;
196+
/// The generic boxing lane for an aggregate with no specialized long/double accessor: an `f16`
197+
/// numeric primitive (whose `SUM` folds through a boxing `doubleValue()`) or a non-numeric column
198+
/// (whose `SUM` is unanswerable and reported `null`). `MIN` / `MAX` order through
199+
/// [Compare#values(Object, Object, DType)].
200+
///
201+
/// @param agg the aggregate column, read through the boxing [Values#valueAt(Array, long)]
202+
/// @param validity the aggregate validity bitmap, or `null` when every row is valid
203+
/// @param aggDtype the aggregate column's dtype, driving the `MIN` / `MAX` order
204+
/// @param numericPrimitive whether the column is a numeric primitive with an answerable `SUM`
205+
/// @param leaves the lowered filter leaves, all of which must accept a row for selection
206+
/// @param n the chunk's row count
207+
/// @return the fold: selected count, non-null count, the [Double] sum (or `null` when non-numeric),
208+
/// and the boxed `MIN` / `MAX` (`null` when no selected row is non-null)
209+
private static FilteredAggregate foldGeneric(Array agg, BoolArray validity, DType aggDtype,
210+
boolean numericPrimitive, RowPredicate[] leaves, long n) {
211+
long selected = 0L;
212+
long nonNull = 0L;
213+
double sum = 0.0;
214+
Object min = null;
215+
Object max = null;
216+
for (long i = 0; i < n; i++) {
217+
if (!allMatch(leaves, i)) {
218+
continue;
219+
}
220+
selected++;
221+
if (validity != null && !validity.getBoolean(i)) {
222+
continue;
223+
}
224+
nonNull++;
225+
Object v = Values.valueAt(agg, i);
226+
if (numericPrimitive) {
227+
sum += ((Number) v).doubleValue();
228+
}
229+
if (min == null) {
230+
min = v;
231+
max = v;
232+
} else {
233+
if (Compare.values(v, min, aggDtype) < 0) {
234+
min = v;
235+
}
236+
if (Compare.values(v, max, aggDtype) > 0) {
237+
max = v;
238+
}
239+
}
190240
}
191-
return new FilteredAggregate(selected, nonNull, sum, min, max);
241+
// The additive identity (0.0) over zero non-null rows for a numeric primitive; null sum otherwise.
242+
return new FilteredAggregate(selected, nonNull, numericPrimitive ? Double.valueOf(sum) : null, min, max);
192243
}
193244

194245
/// Reports whether `a` orders before `b` in the long domain, unsigned-aware so a `u64` value past

‎reader/src/main/java/io/github/dfa1/vortex/reader/compute/FusedFilterSum.java‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -96,12 +96,11 @@ private static Number fused(Array fData, BoolArray fVal, Predicate predicate,
9696
return Long.valueOf(doubleFilterLongAgg(fData, fVal, bound, (LongArray) aData, aVal, n));
9797
}
9898
boolean unsigned = fData.dtype().isUnsigned();
99-
long flip = unsigned ? Long.MIN_VALUE : 0L;
100-
PrimitiveFilter.LongRange range = PrimitiveFilter.lowerLong(predicate, flip);
99+
PrimitiveFilter.LongRange range = PrimitiveFilter.lowerLong(predicate, unsigned ? Long.MIN_VALUE : 0L);
101100
if (aggDouble) {
102-
return Double.valueOf(longFilterDoubleAgg(fData, fVal, range, flip, unsigned, aData, aVal, n));
101+
return Double.valueOf(longFilterDoubleAgg(fData, fVal, range, unsigned, aData, aVal, n));
103102
}
104-
return Long.valueOf(longFilterLongAgg(fData, fVal, range, flip, unsigned, (LongArray) aData, aVal, n));
103+
return Long.valueOf(longFilterLongAgg(fData, fVal, range, unsigned, (LongArray) aData, aVal, n));
105104
}
106105

107106
// -------------------------------------------------------------------------------------------------
@@ -116,14 +115,14 @@ private static Number fused(Array fData, BoolArray fVal, Predicate predicate,
116115
/// @param fData the unwrapped long-domain filter array
117116
/// @param fVal the filter validity bitmap, or `null`
118117
/// @param range the lowered inclusive range on the flipped longs
119-
/// @param flip the order-preserving XOR flip (`Long.MIN_VALUE` unsigned, `0` signed)
120118
/// @param unsigned whether the filter column is unsigned (narrow types zero-extend)
121119
/// @param agg the aggregate array
122120
/// @param aVal the aggregate validity bitmap, or `null`
123121
/// @param n the row count
124122
/// @return the wrapping integer sum over the selected non-null rows
125123
private static long longFilterLongAgg(Array fData, BoolArray fVal, PrimitiveFilter.LongRange range,
126-
long flip, boolean unsigned, LongArray agg, BoolArray aVal, long n) {
124+
boolean unsigned, LongArray agg, BoolArray aVal, long n) {
125+
long flip = unsigned ? Long.MIN_VALUE : 0L;
127126
long lo = range.lo();
128127
long hi = range.hi();
129128
boolean negate = range.negate();
@@ -168,14 +167,14 @@ private static long longFilterLongAgg(Array fData, BoolArray fVal, PrimitiveFilt
168167
/// @param fData the unwrapped long-domain filter array
169168
/// @param fVal the filter validity bitmap, or `null`
170169
/// @param range the lowered inclusive range on the flipped longs
171-
/// @param flip the order-preserving XOR flip
172170
/// @param unsigned whether the filter column is unsigned
173171
/// @param aData the unwrapped double-domain aggregate array
174172
/// @param aVal the aggregate validity bitmap, or `null`
175173
/// @param n the row count
176174
/// @return the floating sum over the selected non-null rows
177175
private static double longFilterDoubleAgg(Array fData, BoolArray fVal, PrimitiveFilter.LongRange range,
178-
long flip, boolean unsigned, Array aData, BoolArray aVal, long n) {
176+
boolean unsigned, Array aData, BoolArray aVal, long n) {
177+
long flip = unsigned ? Long.MIN_VALUE : 0L;
179178
long lo = range.lo();
180179
long hi = range.hi();
181180
boolean negate = range.negate();

0 commit comments

Comments
 (0)