core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/ShortGroupByFunctionBatchTest.java (1)

205-205: Use a non-zero startRowId on the second sum batch to harden API-contract coverage.

On Line 205, passing 0 again doesn’t validate cross-segment offset handling. Using 2 here makes this test consistent with the new segmented-batch contract and better guards future regressions.

Suggested change

-            function.computeBatch(value, ptr, 2, 0);
+            function.computeBatch(value, ptr, 2, 2);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/ShortGroupByFunctionBatchTest.java`
at line 205, In ShortGroupByFunctionBatchTest, the second computeBatch call uses
startRowId 0 which doesn't exercise cross-segment offsets; update the second
invocation of function.computeBatch(value, ptr, 2, 0) to use a non-zero
startRowId (e.g., 2) so it becomes function.computeBatch(value, ptr, 2, 2),
ensuring the computeBatch method is tested with a non-zero startRowId to
validate segmented-batch behavior.

core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java (1)

3132-3137: Update stale inline comment to match the new expectation

Line 3132 says aggregate expressions disable vectorization, but Line 3136 now expects vectorized: true. Please align the comment with the actual behavior to avoid confusion.

✏️ Suggested comment tweak

-    `@Test` // expressions in aggregates disable vectorized impl
+    `@Test` // aggregate expressions may still vectorize depending on function support
     public void testGroupByNotKeyed2() throws Exception {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java` around lines
3132 - 3137, The inline comment before the test method testGroupByNotKeyed2 is
now misleading: it states "expressions in aggregates disable vectorized impl"
while the test expectation shows vectorized: true; update that comment to
reflect the current behavior (e.g., "expressions in aggregates do not disable
vectorized implementation" or similar) so the `@Test` comment matches the expected
output in the test asserting vectorized: true in testGroupByNotKeyed2.

core/src/main/java/io/questdb/griffin/engine/functions/groupby/CountGeoHashGroupByFunctionInt.java (1)

51-51: Skip zero-delta map writes in the batch count path.

Line 51 always performs a map mutation, even when nonNullCount is 0. In a tight vectorized path this adds avoidable write traffic.

♻️ Proposed tweak

-            mapValue.addLong(valueIndex, nonNullCount);
+            if (nonNullCount > 0) {
+                mapValue.addLong(valueIndex, nonNullCount);
+            }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/groupby/CountGeoHashGroupByFunctionInt.java`
at line 51, In CountGeoHashGroupByFunctionInt update the batch count path to
avoid mutating the map when nonNullCount is zero: before calling
mapValue.addLong(valueIndex, nonNullCount) check if nonNullCount != 0 and only
perform the addLong when it's non-zero (leave existing behavior for non-zero
counts); this prevents unnecessary map writes in the tight vectorized path while
preserving current semantics for positive counts.

core/src/main/java/io/questdb/griffin/engine/functions/groupby/MaxDateGroupByFunction.java (1)

48-56: Missing null guard for batchMax – inconsistent with MinLongGroupByFunction.

Vect.maxLong can return Numbers.LONG_NULL when all batch values are null. The current condition batchMax > existing will evaluate to false in that case (since LONG_NULL is Long.MIN_VALUE), so it won't corrupt data. However, this differs from the pattern in MinLongGroupByFunction.computeBatch (lines 51-56) which explicitly guards:

if (batchMin != Numbers.LONG_NULL) {
    // ... update logic
}

Similarly, merge() at line 113 lacks a null guard unlike MinLongGroupByFunction.merge() (line 120).

Consider adding an explicit guard for consistency and clarity:

♻️ Suggested refactor for consistency

     public void computeBatch(MapValue mapValue, long ptr, int count, long startRowId) {
         if (count > 0) {
             final long batchMax = Vect.maxLong(ptr, count);
-            final long existing = mapValue.getDate(valueIndex);
-            if (batchMax > existing) {
-                mapValue.putDate(valueIndex, batchMax);
+            if (batchMax != Numbers.LONG_NULL) {
+                final long existing = mapValue.getDate(valueIndex);
+                if (batchMax > existing || existing == Numbers.LONG_NULL) {
+                    mapValue.putDate(valueIndex, batchMax);
+                }
             }
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/groupby/MaxDateGroupByFunction.java`
around lines 48 - 56, Add explicit null checks like MinLongGroupByFunction: in
MaxDateGroupByFunction.computeBatch, after computing batchMax =
Vect.maxLong(ptr, count) guard with "if (batchMax != Numbers.LONG_NULL)" before
comparing/updating mapValue (use mapValue.getDate(valueIndex) /
mapValue.putDate(valueIndex, batchMax)); likewise update
MaxDateGroupByFunction.merge() to first check merged value != Numbers.LONG_NULL
before merging into existing value to mirror MinLongGroupByFunction's null-guard
pattern for clarity and consistency.

core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/DateGroupByFunctionBatchTest.java (1)

322-347: Strengthen min/max accumulation tests with a non-zero second-batch startRowId.

min/max should be row-id agnostic. Reusing 0 in both calls makes it easier for accidental row-id gating bugs to slip through unnoticed.

🔧 Suggested test hardening

-            function.computeBatch(value, ptr, 2, 0);
+            function.computeBatch(value, ptr, 2, 2);
...
-            function.computeBatch(value, ptr, 2, 0);
+            function.computeBatch(value, ptr, 2, 2);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/DateGroupByFunctionBatchTest.java`
around lines 322 - 347, Update the two tests so the second computeBatch call
uses a non-zero startRowId to ensure row-id agnosticism: in
testMaxDateBatchAccumulates and testMinDateBatchAccumulates change the second
call to MaxDateGroupByFunction.computeBatch(...) and
MinDateGroupByFunction.computeBatch(...) respectively to pass a non-zero
startRowId (e.g., 2) instead of 0 while keeping the batch size (2) unchanged;
this ensures the second batch is treated as a subsequent block of rows rather
than restarting at row 0.

core/src/test/java/io/questdb/test/cairo/fuzz/ParallelGroupByFuzzTest.java (1)

2246-2255: Add plan/path assertions to the two new path-specific tests.

Both tests currently validate only final results. They can still pass after a fallback to non-vectorized/non-hybrid execution, which weakens regression detection for the intended execution paths.

Also applies to: 2323-2333

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/test/java/io/questdb/test/cairo/fuzz/ParallelGroupByFuzzTest.java`
around lines 2246 - 2255, The two new tests
(testParallelNonKeyedGroupByWithColumnTopsAndFirstLast and the similar test at
lines 2323-2333 that call testParallelNonKeyedGroupBy(...)) only validate
results and must also assert the execution path/plan to prevent silent
fallbacks; update each test to capture and assert the query execution plan or
runtime path (e.g., via existing helpers like assertPlanContains or
assertPathUsed) to verify the expected vectorized/hybrid operator is present
(reference the call to testParallelNonKeyedGroupBy and the SQL string passed)
and assert that no non-vectorized/non-hybrid fallback operator is used. Ensure
the assertion checks for the unique operator or plan fragment name your engine
uses for the intended path so the test fails if execution falls back.

core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByNotKeyedAtom.java (1)

97-104: Avoid leaking mutable batch metadata state.

getBatchColumnIndexes() (Line 193) returns the internal array directly, while hasNonBatchFunctions is derived once in constructor (Line 97-104). External mutation can silently desync these two and affect execution-path decisions.

♻️ Suggested defensive copy

@@
 import java.io.Closeable;
+import java.util.Arrays;
@@
-            this.batchColumnIndexes = batchColumnIndexes;
+            this.batchColumnIndexes = Arrays.copyOf(batchColumnIndexes, batchColumnIndexes.length);
@@
     public int[] getBatchColumnIndexes() {
-        return batchColumnIndexes;
+        return Arrays.copyOf(batchColumnIndexes, batchColumnIndexes.length);
     }

Also applies to: 193-195

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByNotKeyedAtom.java`
around lines 97 - 104, Batch metadata can be mutated externally because
batchColumnIndexes is stored directly and getBatchColumnIndexes() exposes the
internal array while hasNonBatchFunctions is computed once in the constructor;
to fix, defensively copy the array both when assigning it in the constructor and
when returning it from getBatchColumnIndexes(), and recompute
hasNonBatchFunctions based on the defensive copy (or compute it from the copied
array) so the internal boolean stays consistent with the stored array; reference
symbols: AsyncGroupByNotKeyedAtom, batchColumnIndexes, hasNonBatchFunctions,
getBatchColumnIndexes().

core/src/main/java/io/questdb/griffin/engine/join/WindowJoinFastRecordCursorFactory.java (1)

889-892: Avoid hardcoding startRowId to 0 in vectorized batch calls.

Line 889 and Line 891 currently force zero-based row ids. Passing a logical window-relative start (e.g., derived from rowLo) makes this path safer if aggregation is ever split into multiple computeBatch() calls per function.

💡 Future-proof wiring

+                    final long batchStartRowId = rowLo;
                     for (int i = 0, n = groupByFunctions.size(); i < n; i++) {
                         int mapIndex = groupByFunctionToColumnIndex.getQuick(i);
                         final long ptr = slaveData.get(idx, mapIndex + 2);
                         if (ptr != 0) {
                             final long typeSize = ColumnType.sizeOfTag((short) groupByFuncTypes.getQuick(mapIndex));
                             // in case of include prevailing column values are shifted by one slot, hence `rowLo + 1`
                             final long efficientRowLo = includePrevailing ? rowLo + 1 : rowLo;
-                            groupByFunctions.getQuick(i).computeBatch(value, columnSink.of(ptr).startAddress() + typeSize * efficientRowLo, (int) (rowHi - rowLo), 0);
+                            groupByFunctions.getQuick(i).computeBatch(value, columnSink.of(ptr).startAddress() + typeSize * efficientRowLo, (int) (rowHi - rowLo), batchStartRowId);
                         } else { // no-arg function, e.g. count()
-                            groupByFunctions.getQuick(i).computeBatch(value, 0, (int) (rowHi - rowLo), 0);
+                            groupByFunctions.getQuick(i).computeBatch(value, 0, (int) (rowHi - rowLo), batchStartRowId);
                         }
                     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/join/WindowJoinFastRecordCursorFactory.java`
around lines 889 - 892, The batch calls to groupByFunctions currently hardcode
startRowId to 0 for no-arg functions, which breaks correctness if aggregation is
split; update the no-arg computeBatch call in WindowJoinFastRecordCursorFactory
so it uses the logical window-relative start (the same start used for the
data-backed path) instead of 0 — e.g., pass the int-casted window start (derived
from rowLo / efficientRowLo) as the third computeBatch startRowId parameter when
calling groupByFunctions.getQuick(i).computeBatch(value, 0/...,
(int)(rowHi-rowLo), startRowId).

core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByNotKeyedRecordCursorFactory.java (2)

282-297: Reduce duplicated row-by-row fallback logic.

The column-top fallback loop in aggregateVect(...) repeats the same row iteration/update pattern already present in aggregate(...). A shared helper would reduce divergence risk between the two paths.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByNotKeyedRecordCursorFactory.java`
around lines 282 - 297, The column-top fallback loop in aggregateVect(...)
duplicates the row-by-row iteration/update logic from aggregate(...); extract
that logic into a shared private helper on
AsyncGroupByNotKeyedRecordCursorFactory (e.g., applyRowByRowFallback or
processFrameRowByRow) that accepts the frameMemory, frameRowCount, slotId, atom,
value, and record (or the minimal pieces needed), moves the
record.init(frameMemory)/record.setRowIndex(0) setup, obtains
GroupByFunctionsUpdater via atom.getFunctionUpdater(slotId), and runs the
for-loop using value.isNew(), functionUpdater.updateNew/updateExisting and
correct rowId incrementing; then replace the duplicated loop in both
aggregateVect(...) and aggregate(...) with a call to this helper so both paths
share identical logic.

---

75-76: Rename boolean field to isVectorized for guideline compliance.

The new boolean field name vectorized should use an is... prefix for consistency with repository conventions.

♻️ Proposed rename

-    private final boolean vectorized;
+    private final boolean isVectorized;
@@
-            this.vectorized = (reducer == AGGREGATE_VECT);
+            this.isVectorized = (reducer == AGGREGATE_VECT);
@@
-        sink.attr("vectorized").val(vectorized);
+        sink.attr("vectorized").val(isVectorized);

As per coding guidelines "When choosing a name for a boolean variable, field or method, always use the is... or has... prefix, as appropriate".

Also applies to: 136-137, 185-185

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByNotKeyedRecordCursorFactory.java`
around lines 75 - 76, Rename the boolean field vectorized in
AsyncGroupByNotKeyedRecordCursorFactory to isVectorized and update all
references (constructor parameter, assignments, any getters or uses) to the new
name; specifically change the private field declaration, the constructor
parameter and its assignment, and any reads of vectorized at locations
referenced (around the areas previously at lines ~136-137 and ~185) to use
isVectorized so the class, its methods and callers compile and follow the is...
boolean naming guideline.

core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/FloatGroupByFunctionBatchTest.java (1)

71-83: Wrap newly added native-memory tests in assertMemoryLeak().

These tests allocate off-heap buffers; adding assertMemoryLeak() would verify cleanup on all paths, including assertion-failure/error paths.

As per coding guidelines "Write all tests using assertMemoryLeak(). This isn't needed for narrow unit tests that don't allocate native memory."

Also applies to: 126-138, 193-205, 222-237, 273-287, 303-315, 350-362, 395-418, 420-432

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/FloatGroupByFunctionBatchTest.java`
around lines 71 - 83, The test allocates off-heap buffers (via allocateFloats
and CountFloatGroupByFunction.computeBatch) but is not wrapped with
assertMemoryLeak(), so native memory might not be verified/cleaned on failure;
update the test method testCountFloatBatchAccumulates (and the other similar
tests in this file that allocate native buffers) to wrap the test body inside
assertMemoryLeak(() -> { ... }), keeping the existing calls to
prepare(function), allocateFloats(...), function.computeBatch(...), and
function.getLong(value) unchanged, and do the same for other
CountFloatGroupByFunction-related test methods referenced in the review (the
blocks around the noted ranges) so all off-heap allocations are checked for
leaks.

core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/GeoHashGroupByFunctionBatchTest.java (1)

82-97: Wrap newly added geohash batch tests with assertMemoryLeak().

These tests rely on native buffers, so leak checks should be enforced at test execution scope, not only in teardown.

As per coding guidelines "Write all tests using assertMemoryLeak(). This isn't needed for narrow unit tests that don't allocate native memory."

Also applies to: 114-130, 146-162, 178-194, 208-221, 246-259, 289-304, 366-381

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/GeoHashGroupByFunctionBatchTest.java`
around lines 82 - 97, Wrap the body of the new geohash batch test methods (e.g.,
testCountGeoHashByteBatchAccumulates) in assertMemoryLeak(() -> { ... }) to
ensure native buffer leak checks run during test execution; keep the existing
resource handling (try-with-resources on SimpleMapValue from prepare(...)) and
calls to allocateBytes(...) and function.computeBatch(...) inside the lambda so
they execute under the leak assertion. Apply the same wrapping to all other
added geohash batch tests referenced (the additional test methods in this file)
so each test runs under assertMemoryLeak.

core/src/test/java/io/questdb/test/griffin/engine/groupby/GroupByFunctionCaseTest.java (1)

245-256: Rename boolean to isAsyncVectorized for guideline compliance.

Line [245] introduces a boolean local without the required is.../has... prefix.

✏️ Proposed refactor

-        boolean asyncVectorized = !rosti && keys == null
+        boolean isAsyncVectorized = !rosti && keys == null
                 && ((t == CHAR && f >= 4)                    // min/max on char
                 || (t == FLOAT && (f == 2 || f >= 4)));      // sum/min/max on float
@@
-                planSink.put("  vectorized: ").put(asyncVectorized).put('\n');
+                planSink.put("  vectorized: ").put(isAsyncVectorized).put('\n');

As per coding guidelines, "When choosing a name for a boolean variable, field or method, always use the is... or has... prefix, as appropriate".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/io/questdb/test/griffin/engine/groupby/GroupByFunctionCaseTest.java`
around lines 245 - 256, Rename the boolean local asyncVectorized to
isAsyncVectorized and update all its usages in this method: replace its
declaration "boolean asyncVectorized = ..." and the reference in the
planSink.put call so the variable name follows the is... prefix guideline;
ensure the boolean expression remains unchanged (still using rosti, keys, t, f)
and run tests to confirm no other references need updating.

core/src/main/java/io/questdb/griffin/engine/functions/groupby/AvgDoubleGroupByFunction.java (1)

55-59: Inconsistent null checks: Numbers.isNull vs Double.isNaN.

Line 53 uses Numbers.isNull(batchSum) while Line 55 uses Double.isNaN(prevSum). For consistency and correctness, consider using the same null-checking approach for both. If Numbers.isNull is the correct check for batch results from Vect.sumDoubleAcc, it should also be used for prevSum.

♻️ Suggested alignment

             if (!Numbers.isNull(batchSum)) {
                 final double prevSum = mapValue.getDouble(valueIndex);
-                if (!Double.isNaN(prevSum)) {
+                if (!Numbers.isNull(prevSum)) {
                     mapValue.putDouble(valueIndex, prevSum + batchSum);
                 } else {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/groupby/AvgDoubleGroupByFunction.java`
around lines 55 - 59, The null-checks in AvgDoubleGroupByFunction are
inconsistent: replace the Double.isNaN(prevSum) check with
Numbers.isNull(prevSum) so both batchSum and prevSum use the same null-detection
(the values produced by Vect.sumDoubleAcc should be tested with Numbers.isNull);
update the branch that writes to mapValue (using valueIndex) to treat a null
prevSum as missing and only set prevSum+batchSum when prevSum is not
Numbers.isNull, otherwise store batchSum, and ensure any existing logic that
relied on Double.isNaN is preserved but using Numbers.isNull instead.

core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java (1)

3132-3136: Update stale test intent comment in testGroupByNotKeyed2.

Line 3132 says expressions disable vectorization, but Line 3136 asserts vectorized: true. Please align the comment with the actual expected behavior to avoid misleading future debugging.

Suggested comment-only fix

-    `@Test` // expressions in aggregates disable vectorized impl
+    `@Test` // mixed aggregate args are expected to run with vectorized=true in this case
     public void testGroupByNotKeyed2() throws Exception {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java` around lines
3132 - 3136, The test comment for testGroupByNotKeyed2 is stale—update the
inline comment that currently reads "expressions in aggregates disable
vectorized impl" to reflect that vectorization is expected (the assertion shows
"vectorized: true"); modify the comment near the test method
testGroupByNotKeyed2 (method name: testGroupByNotKeyed2) so it accurately
describes that expressions in aggregates do not disable vectorized execution in
this case.

core/src/main/java/io/questdb/griffin/engine/functions/groupby/MaxTimestampGroupByFunction.java (1)

48-56: Minor style inconsistency with MinTimestampGroupByFunction.

The logic is correct—LONG_NULL (Long.MIN_VALUE) ensures any valid timestamp will compare as greater. However, MinTimestampGroupByFunction.computeBatch uses explicit null guards (batchMin != Numbers.LONG_NULL), whereas this relies on implicit sentinel comparison. Additionally, computeFirst and setNull use putTimestamp, but computeBatch uses putLong.

Consider aligning the pattern for consistency:

♻️ Suggested alignment (optional)

     public void computeBatch(MapValue mapValue, long ptr, int count, long startRowId) {
         if (count > 0) {
             final long batchMax = Vect.maxLong(ptr, count);
-            final long existing = mapValue.getLong(valueIndex);
-            if (batchMax > existing) {
-                mapValue.putLong(valueIndex, batchMax);
+            if (batchMax != Numbers.LONG_NULL) {
+                final long existing = mapValue.getTimestamp(valueIndex);
+                if (batchMax > existing || existing == Numbers.LONG_NULL) {
+                    mapValue.putTimestamp(valueIndex, batchMax);
+                }
             }
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/groupby/MaxTimestampGroupByFunction.java`
around lines 48 - 56, MaxTimestampGroupByFunction.computeBatch currently relies
on implicit sentinel comparison and uses mapValue.putLong, which is inconsistent
with MinTimestampGroupByFunction and with this class's computeFirst/setNull that
use putTimestamp; update computeBatch in MaxTimestampGroupByFunction to mirror
MinTimestampGroupByFunction by explicitly guarding against Numbers.LONG_NULL for
batchMax (e.g., if (batchMax != Numbers.LONG_NULL) { ... }) and when updating
the map use mapValue.putTimestamp(valueIndex, batchMax) instead of putLong so
the null-sentinel handling and write method are consistent with computeFirst and
setNull.

core/src/main/java/io/questdb/griffin/engine/functions/groupby/CountGeoHashGroupByFunctionByte.java (1)

41-51: Skip zero-delta writes in hot batch path.

This is correct functionally, but writing addLong(..., 0) on null-only batches adds avoidable map churn. Consider guarding on nonNullCount > 0 (as in sibling count implementations).

♻️ Suggested micro-optimization

-            mapValue.addLong(valueIndex, nonNullCount);
+            if (nonNullCount > 0) {
+                mapValue.addLong(valueIndex, nonNullCount);
+            }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/groupby/CountGeoHashGroupByFunctionByte.java`
around lines 41 - 51, In computeBatch of CountGeoHashGroupByFunctionByte, avoid
calling mapValue.addLong(valueIndex, 0) for batches with no non-null bytes to
reduce map churn: after counting nonNullCount (the loop using
Unsafe.getUnsafe().getByte and GeoHashes.BYTE_NULL), only call
mapValue.addLong(valueIndex, nonNullCount) when nonNullCount > 0; leave the
counting loop and variable names (ptr, count, hi, nonNullCount, valueIndex)
unchanged.

core/src/main/java/io/questdb/griffin/engine/functions/groupby/FirstNotNullIPv4GroupByFunctionFactory.java (1)

70-73: Use Integer.BYTES instead of hardcoded 4L in pointer math.

This reduces magic numbers and keeps width semantics tied to type size.

♻️ Suggested cleanup

-                final long hi = ptr + count * 4L;
+                final long hi = ptr + count * (long) Integer.BYTES;
                 long offset = 0;
-                for (; ptr < hi; ptr += 4L) {
+                for (; ptr < hi; ptr += Integer.BYTES) {
                     int value = Unsafe.getUnsafe().getInt(ptr);
                     if (value != Numbers.IPv4_NULL) {
                         long rowId = startRowId + offset;
                         long existingRowId = mapValue.getLong(valueIndex);
                         if (rowId < existingRowId || existingRowId == Numbers.LONG_NULL || mapValue.getIPv4(valueIndex + 1) == Numbers.IPv4_NULL) {
                             mapValue.putLong(valueIndex, rowId);
                             mapValue.putInt(valueIndex + 1, value);
                         }
                         break;
                     }
                     offset++;
                 }

Also applies to: 83-83

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/groupby/FirstNotNullIPv4GroupByFunctionFactory.java`
around lines 70 - 73, Replace the hardcoded 4L used for IPv4 pointer arithmetic
in FirstNotNullIPv4GroupByFunctionFactory with Integer.BYTES to avoid magic
numbers: compute hi as ptr + count * (long)Integer.BYTES, increment ptr by
(long)Integer.BYTES in the loop, and update the similar occurrence at the other
location (around the second loop at the noted lines); keep the getInt(ptr) usage
unchanged but ensure all pointer math consistently uses Integer.BYTES (cast to
long where needed).

core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/DoubleGroupByFunctionBatchTest.java (1)

577-621: Place new testAvg... methods with the other testAvg... members.

The methods added at Line 577+ are out of alphabetical member order relative to the existing testAvg... block. Please move them to preserve class member ordering consistency.

As per coding guidelines: **/*.java: "Group Java class members by kind (static vs. instance) and visibility, sorted alphabetically."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/DoubleGroupByFunctionBatchTest.java`
around lines 577 - 621, The four new test methods
(testAvgDoubleComputeFirstInfinity, testAvgDoubleComputeFirstNaN,
testAvgDoubleComputeNextInfinity, testAvgDoubleComputeNextNaNSkipped) are placed
out of alphabetical order; move these methods inside the existing block of other
testAvg... members in DoubleGroupByFunctionBatchTest so they are grouped with
the other testAvg* methods and sorted alphabetically within that group, keeping
each method signature and body unchanged and preserving the try-with-resources
usage and assertions.

core/src/main/java/io/questdb/griffin/engine/functions/GroupByFunction.java (1)

58-75: Clarify startRowId monotonicity in the computeBatch contract.

The updated docs state multi-call accumulation, but they do not explicitly require increasing, non-overlapping startRowId ranges per group. Making that explicit will prevent incorrect first/last behavior if callers split work into multiple batch invocations.

✏️ Suggested Javadoc clarification

-     * This method may be called multiple times for the same group {`@link` MapValue} (e.g. once
-     * per page frame). Implementations must accumulate into the existing state set by
+     * This method may be called multiple times for the same group {`@link` MapValue} (e.g. once
+     * per page frame). Implementations must accumulate into the existing state set by
      * {`@link` `#setEmpty`(MapValue)}, not overwrite it.
+     * For a given group, repeated calls must use non-overlapping, increasing {`@code` startRowId}
+     * ranges so row-order-aware aggregates (e.g. first/last) remain correct.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/main/java/io/questdb/griffin/engine/functions/GroupByFunction.java`
around lines 58 - 75, Update the Javadoc for GroupByFunction.computeBatch to
explicitly state that for a given MapValue successive invocations of
computeBatch will receive non-overlapping, strictly increasing row ranges: each
call’s startRowId must be greater than the previous call’s startRowId and the
ranges must not overlap, so implementations of computeBatch (which must
accumulate into state set by setEmpty and may be invoked without a preceding
computeFirst or followed by merge when supportsBatchComputation() is true) may
rely on monotonic, non-overlapping batches to implement first/last semantics
correctly.

core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/ShortGroupByFunctionBatchTest.java (2)

230-239: Strengthen accumulation coverage with non-zero second startRowId.

For a true “accumulates across batches” shape under the new API, make the second call use an offset (e.g., 2) rather than 0.

♻️ Suggested test tweak

- ptr = allocateShorts((short) 3, (short) 4);
- function.computeBatch(value, ptr, 2, 0);
+ ptr = allocateShorts((short) 3, (short) 4);
+ function.computeBatch(value, ptr, 2, 2);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/ShortGroupByFunctionBatchTest.java`
around lines 230 - 239, The test currently calls function.computeBatch(value,
ptr, 2, 0) twice, which doesn't verify accumulation when the second batch has a
non-zero startRowId; update the second computeBatch invocation to use a non-zero
offset (e.g., 2) so it reads like function.computeBatch(value, ptr, 2, 2) to
exercise accumulation across batches for SumShortGroupByFunction; keep the rest
of the test (prepare, allocateShorts, SimpleMapValue) unchanged.

---

109-117: Rename *AllNull tests to match actual inputs.

Both methods currently include one non-null value, so names like ...LeadingNull / ...TrailingNull would be less misleading during failure triage.

Also applies to: 187-196

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/ShortGroupByFunctionBatchTest.java`
around lines 109 - 117, The test method names (e.g., testFirstShortBatchAllNull)
are misleading because the test bodies include one non-null value; rename them
to reflect the actual input pattern (e.g., testFirstShortBatchLeadingNull or
testFirstShortBatchTrailingNull) and update the corresponding test at the other
occurrence (lines ~187-196) accordingly; ensure you change the method
declarations and any references to these methods (search for
testFirstShortBatchAllNull and similar) and keep the test logic using
FirstShortGroupByFunction, allocateShorts, and prepare unchanged.

core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByNotKeyedRecordCursorFactory.java (1)

75-75: Rename boolean field to use is... prefix.

The field vectorized at line 75 violates the project convention for boolean naming. Rename to isVectorized to match the guideline: "When choosing a name for a boolean variable, field or method, always use the is... or has... prefix."

♻️ Proposed rename

- private final boolean vectorized;
+ private final boolean isVectorized;
...
- this.vectorized = (reducer == AGGREGATE_VECT);
+ this.isVectorized = (reducer == AGGREGATE_VECT);
...
- sink.attr("vectorized").val(vectorized);
+ sink.attr("vectorized").val(isVectorized);

Applies to lines: 75, 136, 185.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByNotKeyedRecordCursorFactory.java`
at line 75, Rename the boolean field 'vectorized' in
AsyncGroupByNotKeyedRecordCursorFactory to 'isVectorized' and update all
references (constructor parameter, assignments, and any usages such as in
methods or other classes) to the new name; specifically change the field
declaration 'vectorized' to 'isVectorized', update the constructor parameter and
assignment that set this vector flag, and replace all usages at the sites
referenced (previously at lines around 136 and 185) to use 'isVectorized' so
naming follows the is... boolean convention.

core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/GeoHashGroupByFunctionBatchTest.java (1)

366-397: Add last_not_null accumulation tests for BYTE/SHORT/LONG geohash variants too.

This file now validates non-zero-offset accumulation for the INT variant, but the production change touched BYTE/SHORT/LONG as well. Add sibling two-batch tests (startRowId > 0 on the second batch) for those variants to catch type-specific row-id ordering regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/GeoHashGroupByFunctionBatchTest.java`
around lines 366 - 397, Add sibling two-batch unit tests for the BYTE and SHORT
geohash variants (and ensure the LONG variant has a two-batch accumulation test
with a non-zero second batch startRowId) similar to
testLastNotNullGeoHashBatchIntAccumulates: create tests (e.g.,
testLastNotNullGeoHashBatchByteAccumulates and
testLastNotNullGeoHashBatchShortAccumulates) that build a GroupByFunction via
newLastNotNullGeoHashFunction(GeoByteColumn.newInstance(...)) /
GeoShortColumn.newInstance(...), call function.setNull(value), then call
function.computeBatch(value, ptr, len, 0) for the first batch and
function.computeBatch(value, ptr2, len2, startRowId>0) for the second batch
using the appropriate allocateBytes/allocateShorts (and for LONG ensure a
second-batch startRowId>0 case if missing) with GeoHashes.*_NULL/values, and
finally assert the expected value with
function.getGeoByte/getGeoShort/getGeoLong and that
function.supportsBatchComputation() where appropriate.

core/src/test/java/io/questdb/test/griffin/GroupByTest.java (1)

2984-3006: Add plan-shape assertions to lock vectorized execution in these scenarios.

Lines 2995, 3025, 3043, and 3154 currently assert only final results. If planner routing regresses to a non-vectorized path, these tests can still pass and miss the performance regression this change targets.

Suggested pattern (example for one test)

 `@Test`
 public void testNonKeyedVectorizedAllNulls() throws Exception {
     assertMemoryLeak(() -> {
         execute("CREATE TABLE t (v INT, ts TIMESTAMP) TIMESTAMP(ts) PARTITION BY DAY");
         execute(
                 """
                         INSERT INTO t(ts) VALUES
                         ('2024-01-01T00:00:00.000000Z'),
                         ('2024-01-02T00:00:00.000000Z')"""
         );
+        String query = "SELECT sum(v), min(v), max(v), count(*), avg(v), first(v), last(v), first_not_null(v), last_not_null(v) FROM t";
+        assertPlanNoLeakCheck(
+                query,
+                """
+                        Async Group By workers: 1
+                          vectorized: true
+                          values: [sum(v),min(v),max(v),count(*),avg(v),first(v),last(v),first_not_null(v),last_not_null(v)]
+                          filter: null
+                            PageFrame
+                                Row forward scan
+                                Frame forward scan on: t
+                        """
+        );
         assertQueryNoLeakCheck(
                 """
                         sum\tmin\tmax\tcount\tavg\tfirst\tlast\tfirst_not_null\tlast_not_null
                         null\tnull\tnull\t2\tnull\tnull\tnull\tnull\tnull
                         """,
-                "SELECT sum(v), min(v), max(v), count(*), avg(v), first(v), last(v), first_not_null(v), last_not_null(v) FROM t",
+                query,
                 null,
                 false,
                 true
         );
     });
 }

Also applies to: 3008-3036, 3038-3054, 3136-3165

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/test/java/io/questdb/test/griffin/GroupByTest.java` around lines
2984 - 3006, The tests (e.g., GroupByTest.testNonKeyedVectorizedAllNulls)
currently assert only query results; add an assertion that the planner chooses
the vectorized execution path by running an EXPLAIN (or planner plan) for the
same query and asserting the plan text contains the vectorized operator
identifier used by the engine (e.g., "VectorizedGroupBy" or "vectorized") before
calling assertQueryNoLeakCheck; apply the same pattern to the other affected
tests (the tests around the shown ranges that use the same GROUP BY aggregate
SQL and assertQueryNoLeakCheck) so they fail if the planner regresses to a
non-vectorized plan.