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

103-109: Consider clearing partitionByRecord functions on close.

To mirror the ObjList hygiene fix elsewhere and reduce chances of iterating nulls post-close, prefer freeObjListAndClear(...).

-                Misc.freeObjList(partitionByRecord.getFunctions());
+                Misc.freeObjListAndClear(partitionByRecord.getFunctions());

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

187-189: Readability nit: simplify zero-row window check.

-                else if (rowsLo == 0 && rowsLo == rowsHi) {
+                else if (rowsLo == 0 && rowsHi == 0) {

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

187-189: Readability nit: simplify zero-row window check.

-                else if (rowsLo == 0 && rowsLo == rowsHi) {
+                else if (rowsLo == 0 && rowsHi == 0) {

core/src/main/java/io/questdb/griffin/engine/functions/window/MaxTimestampWindowFunctionFactory.java (3)

462-463: Typo in comment: "undobuned" should be "unbounded"

-    // Handles max() over (partition by x order by ts range between [undobuned | y] preceding and [z preceding | current row])
+    // Handles max() over (partition by x order by ts range between [unbounded | y] preceding and [z preceding | current row])

---

441-444: Consider using putLong for consistency

The code uses putTimestamp to store a long value, while other similar implementations use putLong. While both work for TIMESTAMP columns, using putLong would be more consistent with the rest of the codebase.

-                    if (comparator.compare(l, value.getTimestamp(0))) {
-                        value.putTimestamp(0, l);
+                    if (comparator.compare(l, value.getLong(0))) {
+                        value.putLong(0, l);
                     }
                 } else {
-                    value.putTimestamp(0, l);
+                    value.putLong(0, l);

---

1676-1676: Inconsistency in column type definition

The MAX_COLUMN_TYPES uses ColumnType.TIMESTAMP to store the max value, but the variable name and usage suggest it's storing a long value. This could be confusing for maintainers.

-        MAX_COLUMN_TYPES.add(ColumnType.TIMESTAMP); // max value
+        MAX_COLUMN_TYPES.add(ColumnType.LONG); // max value

core/src/main/java/io/questdb/griffin/engine/functions/window/MaxLongWindowFunctionFactory.java (4)

462-463: Typo in comment: "undobuned" should be "unbounded"

-    // Handles max() over (partition by x order by ts range between [undobuned | y] preceding and [z preceding | current row])
+    // Handles max() over (partition by x order by ts range between [unbounded | y] preceding and [z preceding | current row])

---

441-444: Inconsistent method usage: putTimestamp vs putLong

In MaxOverPartitionFunction, you're using putTimestamp to store a long value when the function returns long. This is inconsistent with other implementations.

                 if (!value.isNew()) {
                     if (comparator.compare(l, value.getLong(0))) {
-                        value.putTimestamp(0, l);
+                        value.putLong(0, l);
                     }
                 } else {
-                    value.putTimestamp(0, l);
+                    value.putLong(0, l);

---

518-519: Inconsistent use of putTimestamp with long value

Similar inconsistency in MaxMinOverUnboundedPartitionRowsFrameFunction.

                     long max = value.getLong(0);
                     if (comparator.compare(l, max)) {
-                        value.putTimestamp(0, l);
+                        value.putLong(0, l);
                         max = l;

---

882-882: Potential refactoring opportunity for map value field access

In MaxMinOverPartitionRowsFrameFunction, when frameLoBounded is false, you're using value.getTimestamp(2) and value.putTimestamp(2, max) to store a long value at index 2. Consider using putLong/getLong for consistency.

                 } else {
-                    value.putTimestamp(2, this.maxMin);
+                    value.putLong(2, this.maxMin);
                 }

                     long max = value.getLong(2);
                     if (hiValue != Numbers.LONG_NULL) {
                         if (max == Numbers.LONG_NULL || comparator.compare(hiValue, max)) {
                             max = hiValue;
-                            value.putTimestamp(2, max);
+                            value.putLong(2, max);
                         }

Also applies to: 914-914

core/src/main/java/io/questdb/griffin/engine/functions/window/LastValueLongWindowFunctionFactory.java (2)

2031-2031: Typo in plan output: "between and" should be "and"

-                sink.val(" preceding between and ");
+                sink.val(" preceding and ");

---

601-604: Inconsistent use of putTimestamp with long values

Similar to MaxLongWindowFunctionFactory, there's inconsistent use of putTimestamp to store long values throughout this file.

For example:

                 if (d != Numbers.LONG_NULL) {
                     MapValue value = key.createValue();
-                    value.putTimestamp(0, d);
+                    value.putLong(0, d);
                 }

-            long val = value != null ? value.getTimestamp(0) : Numbers.LONG_NULL;
+            long val = value != null ? value.getLong(0) : Numbers.LONG_NULL;

Also applies to: 614-614

core/src/main/java/io/questdb/griffin/engine/functions/window/LastValueTimestampWindowFunctionFactory.java (2)

954-956: Nit: simplify ring-buffer copy length expression.

((firstIdx + size) % size) equals firstIdx; use firstIdx for clarity.

-                        Vect.memcpy(newAddress + firstPieceSize, oldAddress, ((firstIdx + size) % size) * RECORD_SIZE);
+                        Vect.memcpy(newAddress + firstPieceSize, oldAddress, firstIdx * RECORD_SIZE);

And similarly in LastValueOverRangeFrameFunction.

Also applies to: 1871-1873

---

686-693: Consider overflow-safe timestamp diffs.

Using Math.abs(timestamp - ts) can overflow for extreme values. Prefer a safe abs-diff helper if available (e.g., Numbers.absDiff or equivalent).

Also applies to: 716-723, 1839-1847

core/src/main/java/io/questdb/griffin/engine/functions/window/FirstValueLongWindowFunctionFactory.java (2)

215-225: Fix column-metadata comments to match actual usage.

Map layout here is: 0=oldest idx, 1=start offset, 2=first-not-null idx, 3=count (unbounded). Comments currently list "count" twice.

-                    columnTypes.add(ColumnType.LONG);// position of current oldest element
-                    columnTypes.add(ColumnType.LONG);// start offset of native array
-                    columnTypes.add(ColumnType.LONG);// count of values in buffer
-                    columnTypes.add(ColumnType.LONG);// count of values in buffer
+                    columnTypes.add(ColumnType.LONG);// position of current oldest element
+                    columnTypes.add(ColumnType.LONG);// start offset of native array
+                    columnTypes.add(ColumnType.LONG);// first not-null index
+                    columnTypes.add(ColumnType.LONG);// count (only when frameLoUnBounded)

---

167-168: Nit: update copy/paste comments ("moving average") to "first_value".

These comments are misleading.

-                    // moving average over range between timestamp - rowsLo and timestamp + rowsHi (inclusive)
+                    // first_value over range between timestamp - rowsLo and timestamp + rowsHi (inclusive)

-                    // moving average over range between timestamp - rowsLo and timestamp + rowsHi (inclusive)
+                    // first_value over range between timestamp - rowsLo and timestamp + rowsHi (inclusive)

-                    // moving average over preceding N rows
+                    // first_value over preceding N rows

Also applies to: 372-373, 437-439

core/src/main/java/io/questdb/griffin/engine/functions/window/FirstValueTimestampWindowFunctionFactory.java (3)

981-986: Prefer putTimestamp for TIMESTAMP map slots (clarity).

Semantics are identical, but putTimestamp improves readability and type intent.

-                    mapValue.putLong(0, d);
+                    mapValue.putTimestamp(0, d);

-                value.putLong(0, firstValue);
+                value.putTimestamp(0, firstValue);

-                mapValue.putLong(0, d);
+                mapValue.putTimestamp(0, d);

Also applies to: 1116-1121, 1893-1896

---

167-168: Nit: fix "moving average" copy/paste comments.

Use "first_value" to avoid confusion.

-                    // moving average over range between timestamp - rowsLo and timestamp + rowsHi (inclusive)
+                    // first_value over range between timestamp - rowsLo and timestamp + rowsHi (inclusive)

-                    // moving average over range between timestamp - rowsLo and timestamp + rowsHi (inclusive)
+                    // first_value over range between timestamp - rowsLo and timestamp + rowsHi (inclusive)

-                    // moving average over preceding N rows
+                    // first_value over preceding N rows

Also applies to: 372-373, 437-438

---

805-813: Consider overflow-safe timestamp diffs.

Same note as in last_value: Math.abs(timestamp - ts) can overflow; prefer a safe abs-diff helper.

Also applies to: 829-836, 1596-1610

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

core/src/main/java/io/questdb/griffin/engine/functions/window/MinTimestampWindowFunctionFactory.java (5)

313-341: Constructor not guarded; mem/dequeMem leak if it throws.

Wrap the constructor along with allocations. This repeats a prior comment.

-                    MemoryARW mem = null;
-                    MemoryARW dequeMem = null;
-                    try {
-                        mem = Vm.getCARWInstance(
-                                configuration.getSqlWindowStorePageSize(),
-                                configuration.getSqlWindowStoreMaxPages(),
-                                MemoryTag.NATIVE_CIRCULAR_BUFFER
-                        );
-                        if (rowsLo != Long.MIN_VALUE) {
-                            dequeMem = Vm.getCARWInstance(
-                                    configuration.getSqlWindowStorePageSize(),
-                                    configuration.getSqlWindowStoreMaxPages(),
-                                    MemoryTag.NATIVE_CIRCULAR_BUFFER
-                            );
-                        }
-                    } catch (Throwable e) {
-                        Misc.free(mem);
-                        Misc.free(dequeMem);
-                        throw e;
-                    }
-                    return new MaxTimestampWindowFunctionFactory.MaxMinOverRowsFrameFunction(
-                            args.get(0),
-                            rowsLo,
-                            rowsHi,
-                            mem,
-                            dequeMem,
-                            LESS_THAN,
-                            NAME
-                    );
+                    MemoryARW mem = null;
+                    MemoryARW dequeMem = null;
+                    try {
+                        mem = Vm.getCARWInstance(
+                                configuration.getSqlWindowStorePageSize(),
+                                configuration.getSqlWindowStoreMaxPages(),
+                                MemoryTag.NATIVE_CIRCULAR_BUFFER
+                        );
+                        if (rowsLo != Long.MIN_VALUE) {
+                            dequeMem = Vm.getCARWInstance(
+                                    configuration.getSqlWindowStorePageSize(),
+                                    configuration.getSqlWindowStoreMaxPages(),
+                                    MemoryTag.NATIVE_CIRCULAR_BUFFER
+                            );
+                        }
+                        return new MaxTimestampWindowFunctionFactory.MaxMinOverRowsFrameFunction(
+                                args.get(0),
+                                rowsLo,
+                                rowsHi,
+                                mem,
+                                dequeMem,
+                                LESS_THAN,
+                                NAME
+                        );
+                    } catch (Throwable th) {
+                        Misc.free(mem);
+                        Misc.free(dequeMem);
+                        throw th;
+                    }

---

191-205: Same leak pattern: protect map allocation/constructor.

Wrap to ensure map is freed on constructor failure.

-                    Map map = MapFactory.createUnorderedMap(
-                            configuration,
-                            partitionByKeyTypes,
-                            MaxTimestampWindowFunctionFactory.MAX_COLUMN_TYPES
-                    );
-
-                    return new MaxTimestampWindowFunctionFactory.MaxMinOverPartitionFunction(
-                            map,
-                            partitionByRecord,
-                            partitionBySink,
-                            args.get(0),
-                            LESS_THAN,
-                            NAME
-                    );
+                    Map map = null;
+                    try {
+                        map = MapFactory.createUnorderedMap(
+                                configuration,
+                                partitionByKeyTypes,
+                                MaxTimestampWindowFunctionFactory.MAX_COLUMN_TYPES
+                        );
+                        return new MaxTimestampWindowFunctionFactory.MaxMinOverPartitionFunction(
+                                map,
+                                partitionByRecord,
+                                partitionBySink,
+                                args.get(0),
+                                LESS_THAN,
+                                NAME
+                        );
+                    } catch (Throwable th) {
+                        Misc.free(map);
+                        throw th;
+                    }

---

86-100: Free map on constructor failure (possible native leak).

Wrap map allocation + constructor in try/catch.

-                    Map map = MapFactory.createUnorderedMap(
-                            configuration,
-                            partitionByKeyTypes,
-                            MaxTimestampWindowFunctionFactory.MAX_COLUMN_TYPES
-                    );
-
-                    return new MaxTimestampWindowFunctionFactory.MaxMinOverPartitionFunction(
-                            map,
-                            partitionByRecord,
-                            partitionBySink,
-                            args.get(0),
-                            LESS_THAN,
-                            NAME
-                    );
+                    Map map = null;
+                    try {
+                        map = MapFactory.createUnorderedMap(
+                                configuration,
+                                partitionByKeyTypes,
+                                MaxTimestampWindowFunctionFactory.MAX_COLUMN_TYPES
+                        );
+                        return new MaxTimestampWindowFunctionFactory.MaxMinOverPartitionFunction(
+                                map,
+                                partitionByRecord,
+                                partitionBySink,
+                                args.get(0),
+                                LESS_THAN,
+                                NAME
+                        );
+                    } catch (Throwable th) {
+                        Misc.free(map);
+                        throw th;
+                    }

---

102-116: Same leak pattern: free map if constructor throws.

Apply the same try/catch as above.

-                    Map map = MapFactory.createUnorderedMap(
-                            configuration,
-                            partitionByKeyTypes,
-                            MaxTimestampWindowFunctionFactory.MAX_COLUMN_TYPES
-                    );
-
-                    return new MaxTimestampWindowFunctionFactory.MaxMinOverUnboundedPartitionRowsFrameFunction(
-                            map,
-                            partitionByRecord,
-                            partitionBySink,
-                            args.get(0),
-                            LESS_THAN,
-                            NAME
-                    );
+                    Map map = null;
+                    try {
+                        map = MapFactory.createUnorderedMap(
+                                configuration,
+                                partitionByKeyTypes,
+                                MaxTimestampWindowFunctionFactory.MAX_COLUMN_TYPES
+                        );
+                        return new MaxTimestampWindowFunctionFactory.MaxMinOverUnboundedPartitionRowsFrameFunction(
+                                map,
+                                partitionByRecord,
+                                partitionBySink,
+                                args.get(0),
+                                LESS_THAN,
+                                NAME
+                        );
+                    } catch (Throwable th) {
+                        Misc.free(map);
+                        throw th;
+                    }

---

171-186: Same leak pattern: protect map allocation/constructor.

Wrap to avoid leaking native map if constructor throws.

-                    Map map = MapFactory.createUnorderedMap(
-                            configuration,
-                            partitionByKeyTypes,
-                            MaxTimestampWindowFunctionFactory.MAX_COLUMN_TYPES
-                    );
-
-                    return new MaxTimestampWindowFunctionFactory.MaxMinOverUnboundedPartitionRowsFrameFunction(
-                            map,
-                            partitionByRecord,
-                            partitionBySink,
-                            args.get(0),
-                            LESS_THAN,
-                            NAME
-                    );
+                    Map map = null;
+                    try {
+                        map = MapFactory.createUnorderedMap(
+                                configuration,
+                                partitionByKeyTypes,
+                                MaxTimestampWindowFunctionFactory.MAX_COLUMN_TYPES
+                        );
+                        return new MaxTimestampWindowFunctionFactory.MaxMinOverUnboundedPartitionRowsFrameFunction(
+                                map,
+                                partitionByRecord,
+                                partitionBySink,
+                                args.get(0),
+                                LESS_THAN,
+                                NAME
+                        );
+                    } catch (Throwable th) {
+                        Misc.free(map);
+                        throw th;
+                    }

core/src/main/java/io/questdb/griffin/engine/functions/window/MinTimestampWindowFunctionFactory.java (2)

306-308: Nit: readability on zero-row frame check.

Use rowsHi == 0 for clarity.

-                else if (rowsLo == 0 && rowsLo == rowsHi) {
+                else if (rowsLo == 0 && rowsHi == 0) {

Also applies to: 186-189

---

46-50: Optional: add minimal Javadoc.

Short class/function Javadoc improves discoverability in IDEs.

 public class MinTimestampWindowFunctionFactory extends AbstractWindowFunctionFactory {
+    /**
+     * MIN over TIMESTAMP window function factory.
+     * Mirrors MAX implementation with LESS_THAN comparator.
+     */

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro