core/src/main/java/io/questdb/std/datetime/AbstractDateFormat.java (1)

30-32: Good: Javadoc added to abstract class.

The documentation is clear and syntactically correct. Consider expanding it with implementation guidance or behavioral notes for subclasses if needed—e.g., details on parse semantics or thread-safety expectations.

core/src/main/java/io/questdb/cairo/AbstractFullPartitionFrameCursor.java (1)

34-128: Documentation additions look good.

The Javadoc comments added throughout this file are accurate and follow standard conventions. While they are somewhat terse (mostly restating field/method names), they provide a baseline level of documentation that improves code readability.

Optional: Consider enriching documentation for better maintainability

If you'd like to enhance these docs further, consider adding:

• Context and purpose: Explain what partition frames represent and how they're used in the cursor lifecycle
• Invariants: Document relationships between fields (e.g., partitionIndex range relative to partitionHi)
• Thread safety: Note any concurrency expectations for readers using this cursor
• Lifecycle: Clarify when fields are initialized and their validity scope

Example for partitionHi:

 /**
- * The partition high boundary.
+ * The partition high boundary (exclusive upper bound).
+ * Represents the total number of partitions in the reader.
+ * Valid after of(TableReader) is called; updated on reload().
  */
 protected int partitionHi;

This is purely optional and can be deferred to future documentation improvements.

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

38-48: Documentation added for class and fields.

The JavaDoc additions follow standard conventions and improve code maintainability. The documentation is functional but could be more descriptive to provide additional context (e.g., explaining what kind of function arg represents in count operations, or which map valueIndex refers to).

core/src/main/java/io/questdb/cutlass/http/client/Fragment.java (1)

27-43: Consider enhancing the documentation with additional context.

The added Javadoc is clear but minimal. For better developer experience, consider documenting:

• What the memory addresses represent (raw pointers, buffer offsets, etc.)
• Thread-safety guarantees
• Lifecycle and validity (when is the Fragment valid? who owns the memory?)
• Relationship to Response (since Fragment is returned by Response.recv())

📝 Example enhanced documentation

 /**
- * Represents a fragment of data with low and high memory addresses.
+ * Represents a contiguous fragment of HTTP response data in native memory.
+ * The fragment is valid only until the next call to Response.recv() or until
+ * the Response is closed. The memory addresses are raw pointers to native memory
+ * managed by the HTTP client.
+ * <p>
+ * This interface is not thread-safe and should only be accessed from the thread
+ * that received it from Response.recv().
  */
 public interface Fragment {
     /**
-     * Returns the high address of this fragment.
+     * Returns the exclusive upper bound address of this fragment in native memory.
      *
-     * @return the high address
+     * @return the high address (exclusive)
      */
     long hi();

     /**
-     * Returns the low address of this fragment.
+     * Returns the inclusive lower bound address of this fragment in native memory.
      *
-     * @return the low address
+     * @return the low address (inclusive)
      */
     long lo();
 }

core/src/main/java/io/questdb/cutlass/http/client/Response.java (1)

31-44: Consider enhancing the method documentation.

The documentation for both recv() methods could be more comprehensive. Consider adding:

• What the "default timeout" value is for recv()
• What happens when a timeout occurs (exception thrown? null returned? special Fragment value?)
• Whether timeout values of 0 or negative numbers have special meaning
• Thread-safety guarantees
• Whether Fragment can be null on error conditions

📝 Example enhanced documentation

     /**
-     * Receives the next fragment of response data using the default timeout.
+     * Receives the next fragment of response data using the default timeout (5000ms).
+     * Blocks until data is available or the timeout expires.
      *
-     * @return the received fragment
+     * @return the received fragment, never null
+     * @throws io.questdb.cairo.HttpException if the timeout expires or connection fails
      */
     Fragment recv();

     /**
-     * Receives the next fragment of response data with the specified timeout.
+     * Receives the next fragment of response data with the specified timeout.
+     * Blocks until data is available or the timeout expires.
      *
-     * @param timeout the timeout in milliseconds
-     * @return the received fragment
+     * @param timeout the timeout in milliseconds (0 = no timeout, negative = default)
+     * @return the received fragment, never null
+     * @throws io.questdb.cairo.HttpException if the timeout expires or connection fails
+     * @throws IllegalArgumentException if timeout is negative and not a special value
      */
     Fragment recv(int timeout);

core/src/main/java/io/questdb/griffin/engine/groupby/vect/AbstractCountVectorAggregateFunction.java (1)

57-61: Consider using private final fields or adding null checks for protected fields used by subclasses.

The fields distinctFunc and keyValueFunc are declared in this abstract class but never referenced in its methods. Subclasses like CountIntVectorAggregateFunction assign these fields in their constructors and then use them directly in aggregate() without null checks. While the current Count* subclasses properly initialize these fields in all code paths, this pattern is error-prone: if a subclass forgets to initialize either field, an NPE will occur at runtime with no way to detect it in the abstract class. Other similar aggregate functions in this package use private final fields instead, which is more explicit and prevents accidental misuse by subclasses.

core/src/main/java/io/questdb/std/AbstractCharSequenceHashSet.java (1)

120-128: Consider defensive validation in keyAt.

The method assumes index is negative (as documented), but passing a non-negative index would cause incorrect array access. Adding a precondition check would improve safety:

public CharSequence keyAt(int index) {
    assert index < 0 : "index must be negative";
    return keys[-index - 1];
}

However, this is a performance tradeoff since the negative-index convention is already documented.

core/src/test/java/io/questdb/test/ServerMainTest.java (1)

127-202: Well-structured hydration test with minor robustness suggestion.

The test correctly validates RecentWriteTracker hydration across server restarts. The setup with wait_wal_table() ensures deterministic state, and setting the hydration callback before start() is the correct order.

One minor robustness concern: hydrationLatch.await() on line 181 has no timeout, which could cause the test to hang indefinitely if hydration fails unexpectedly. Consider adding a timeout variant if SOCountDownLatch supports it.

🔎 Optional: Add timeout to prevent test hanging

If SOCountDownLatch supports a timed await, consider:

// Wait for hydration with timeout to prevent indefinite hang
boolean completed = hydrationLatch.await(30, TimeUnit.SECONDS);
Assert.assertTrue("Hydration should complete within timeout", completed);

core/src/main/java/io/questdb/cairo/wal/ApplyWal2TableJob.java (2)

522-522: Simplify redundant double rounding.

Line 522 applies Numbers.roundUp twice to the same value, which is redundant. The expression Numbers.roundUp(Numbers.roundUp(100.0 * physicalRowsAdded / rowsAdded, 2) / 100.0, 2) can be simplified.

🔎 Proposed simplification

-                    double amplification = rowsAdded > 0 ? Numbers.roundUp(Numbers.roundUp(100.0 * physicalRowsAdded / rowsAdded, 2) / 100.0, 2) : 0;
+                    double amplification = rowsAdded > 0 ? Numbers.roundUp((double) physicalRowsAdded / rowsAdded, 2) : 0;

---

660-662: Consider wrapping tracker call in try-catch for consistency.

The recordWalProcessed call is not wrapped in a try-catch block, unlike the similar recordWrite call in WriterPool.java (lines 577-582). If tracking fails here, it could disrupt WAL processing.

🔎 Proposed defensive wrapping

-                // Decrement pending WAL row count and track dedup after successful processing
-                engine.getRecentWriteTracker().recordWalProcessed(writer.getTableToken(), lastCommittedSeqTxn, lastCommittedRows, rowsCommitted);
-
+                // Decrement pending WAL row count and track dedup after successful processing
+                try {
+                    engine.getRecentWriteTracker().recordWalProcessed(writer.getTableToken(), lastCommittedSeqTxn, lastCommittedRows, rowsCommitted);
+                } catch (Throwable th) {
+                    LOG.error().$("failed to track WAL processing [table=").$(writer.getTableToken())
+                            .$(", error=").$(th).I$();
+                }
+

core/src/main/java/io/questdb/PropServerConfiguration.java (1)

391-391: RecentWriteTracker capacity wiring looks consistent; optional validation

The new recentWriteTrackerCapacity field is read from CAIRO_RECENT_WRITE_TRACKER_CAPACITY with a default of 1000 and exposed via PropCairoConfiguration.getRecentWriteTrackerCapacity(), which aligns with the tracker’s configuration needs.

If RecentWriteTracker does not explicitly handle non‑positive capacities, consider validating (e.g., > 0) here and throwing a ServerConfigurationException on invalid values to fail fast with a clearer message rather than deferring to deeper runtime failures.

Also applies to: 1416-1417, 3658-3661

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

35-157: BinaryFunction default behavior is solid; be mindful of child contracts and commutativity

Centralizing binary-function lifecycle and state (init/close/memoize/toPlan/etc.) here is a good cleanup and should significantly reduce duplication across binary function implementations.

Two soft-contract points to keep in mind for implementors:

• getLeft() / getRight() are implicitly assumed non‑null and independently closeable; if any implementation ever reuses the same Function instance for both sides, that child will see close() / cursorClosed() twice. Either avoid such sharing or ensure those children are idempotent.
• isEquivalentTo is structurally order-sensitive; for truly commutative operators that should treat a op b equivalent to b op a, consider overriding isEquivalentTo in those specific implementations.

Otherwise, the defaults (especially for constantness, runtime-constant detection, and parallelism flags) look consistent with existing Function semantics.

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

32-73: AbstractEqBinaryFunction cleanly integrates equality functions with BinaryFunction

The new base class correctly encapsulates left/right operands, exposes them via getLeft()/getRight(), and provides a negation-aware toPlan that yields a=b or a!=b as expected. This should simplify equality-function implementations and make them benefit from BinaryFunction’s shared lifecycle and state-handling defaults without extra boilerplate.

If you later want to rely on BinaryFunction’s generic operator rendering, you could override isOperator() to return true and delegate to the default toPlan, but the dedicated toPlan here is perfectly fine and explicit.

core/src/test/java/io/questdb/test/cairo/pool/RecentWriteTrackerTest.java (1)

207-227: Eviction test assertion message vs bound is slightly inconsistent

In testEviction, the comment and failure message say “size should be around capacity (5)” / “<= capacity after eviction”, but the assertion uses tracker.size() <= 10, effectively allowing up to 2 * capacity to match the current implementation’s eviction threshold.

To avoid confusion (and future maintenance mistakes if the eviction policy changes), consider:

• Updating the comment and assertion message to explicitly say “<= 2x capacity”, or
• Deriving the bound from the configured capacity (e.g., 2 * capacity) instead of the literal 10.

benchmarks/src/main/java/org/questdb/RecentWriteTrackerBenchmark.java (1)

207-221: Consider adding a note clarifying the simplified WriteStats design.

The benchmark's WriteStats is intentionally simplified compared to the production RecentWriteTracker.WriteStats (which uses AtomicLong, LongAdder, histograms, and locks). Since the benchmark targets lambda allocation patterns rather than full stat tracking semantics, this is appropriate, but a brief comment would help future readers understand this design choice.

Suggested documentation

-    // Simple WriteStats class for benchmarking
+    // Simplified WriteStats class for benchmarking allocation patterns.
+    // The production RecentWriteTracker.WriteStats uses atomic fields, histograms,
+    // and locks - omitted here since we're only measuring lambda capture overhead.
     public static class WriteStats {

core/src/test/java/io/questdb/test/griffin/engine/functions/catalogue/TablesFunctionFactoryTest.java (2)

218-226: Multiline string literal formatting could be clearer.

The embedded writerTxn value concatenation within the multiline string creates unusual formatting. Consider using String.format or a simpler concatenation approach for better readability.

🔎 Suggested improvement

-            assertSql(
-                    """
-                            table_name\twriterTxn\tsequencerTxn\tlastWalTimestamp
-                            test_non_wal\t""" + writerTxn + """
-                            \tnull\t
-                            """,
-                    "select table_name, writerTxn, sequencerTxn, lastWalTimestamp from tables() where table_name = 'test_non_wal'"
-            );
+            assertSql(
+                    "table_name\twriterTxn\tsequencerTxn\tlastWalTimestamp\n" +
+                    "test_non_wal\t" + writerTxn + "\tnull\t\n",
+                    "select table_name, writerTxn, sequencerTxn, lastWalTimestamp from tables() where table_name = 'test_non_wal'"
+            );

---

374-382: Similar multiline string formatting issue.

Same readability concern with the embedded values in multiline string literals.

core/src/test/java/io/questdb/test/cairo/pool/RecentWriteTrackerIntegrationTest.java (2)

48-76: Test lacks meaningful assertions.

This test serves as documentation but only asserts that the tracker is not null. Consider either adding substantive assertions or converting this to actual documentation/Javadoc.

---

320-321: Thread.sleep(1) is unreliable for timestamp differentiation.

Sub-millisecond sleeps are not guaranteed by the JVM and may not produce different timestamps on fast systems. The test assertion uses >= which handles this, but the comment suggests the intent is to ensure different timestamps.

🔎 Alternative approach

Consider using a test clock or explicitly advancing time instead of relying on Thread.sleep(1):

// Small delay to ensure different timestamp
// Note: Using >= assertion since Thread.sleep(1) may not guarantee different microsecond timestamps

Or use a more robust approach that doesn't depend on timing at all.

core/src/main/java/io/questdb/cairo/pool/RecentWriteTracker.java (2)

534-542: Recursive quicksort may cause StackOverflowError for large datasets.

With default capacity of 1000 and eviction at 2x (2000 entries), a worst-case sorted input could require ~2000 stack frames. While typical JVM stacks can handle this, consider using an iterative approach or Java's built-in sorting for robustness.

🔎 Alternative using iterative or built-in sort

For robustness, consider using an iterative quicksort with an explicit stack, or sort using indices into an array:

// Option: Use indices array and Arrays.sort with comparator
int[] indices = new int[size];
for (int i = 0; i < size; i++) indices[i] = i;
// Sort indices by timestamps descending

---

494-508: O(n*k) eviction algorithm may cause latency spikes.

When evicting from 2x capacity down to capacity, this iterates the entire map k times (where k = capacity). For the default capacity of 1000, this means 1000 full iterations over 2000 entries = 2M operations. Consider using a min-heap or sorting once.

🔎 More efficient eviction approach

// Instead of k iterations finding minimum each time:
// 1. Collect all entries with timestamps
// 2. Sort once
// 3. Remove oldest k entries

List<Map.Entry<TableToken, WriteStats>> entries = new ArrayList<>(writeStats.entrySet());
entries.sort(Comparator.comparingLong(e -> e.getValue().getMaxTimestamp()));
for (int i = 0; i < toEvict && i < entries.size(); i++) {
    writeStats.remove(entries.get(i).getKey());
}

core/src/main/java/io/questdb/griffin/engine/functions/catalogue/TablesFunctionFactory.java (3)

266-282: Assert statement may cause unexpected failures.

Line 280 uses assert col == MAX_UNCOMMITTED_ROWS_COLUMN after several if statements. If a new INT column is added but not handled, this will fail with assertions enabled. Consider using an explicit else if with a default case.

🔎 Safer pattern

 @Override
 public int getInt(int col) {
     if (col == ID_COLUMN) {
         return table.getId();
     }
     if (col == TTL_VALUE_COLUMN) {
         return getTtlValue(table.getTtlHoursOrMonths());
     }
     if (col == MEMORY_PRESSURE_LEVEL_COLUMN) {
         if (!table.isWalEnabled()) {
             return Numbers.INT_NULL;
         }
         SeqTxnTracker tracker = tableSequencerAPI.getTxnTracker(table.getTableToken());
         return tracker.getMemPressureControl().getMemoryPressureLevel();
     }
-    assert col == MAX_UNCOMMITTED_ROWS_COLUMN;
-    return table.getMaxUncommittedRows();
+    if (col == MAX_UNCOMMITTED_ROWS_COLUMN) {
+        return table.getMaxUncommittedRows();
+    }
+    return Numbers.INT_NULL;
 }

---

284-296: Inconsistent return value for null writeStats.

When writeStats == null, the method returns 0.0, but the default case returns Double.NaN. This inconsistency could confuse consumers. Consider returning Double.NaN consistently for missing data.

🔎 Consistent null handling

 @Override
 public double getDouble(int col) {
     if (writeStats == null) {
-        return 0.0;
+        return Double.NaN;
     }
     return switch (col) {

---

303-313: Complex null-handling condition is hard to maintain.

The long chain of || conditions for determining which columns return 0 vs Numbers.LONG_NULL is error-prone when adding new columns. Consider grouping related columns or using a Set for cleaner logic.

🔎 Alternative approach using column groups

// Define sets of columns that return 0 when writeStats is null
private static final IntHashSet ZERO_WHEN_NULL_COLUMNS = new IntHashSet();
static {
    ZERO_WHEN_NULL_COLUMNS.add(PENDING_ROW_COUNT_COLUMN);
    ZERO_WHEN_NULL_COLUMNS.add(DEDUPE_ROW_COUNT_COLUMN);
    ZERO_WHEN_NULL_COLUMNS.add(TXN_COUNT_COLUMN);
    // ... etc
}

// Then in getLong:
if (writeStats == null) {
    return ZERO_WHEN_NULL_COLUMNS.contains(col) ? 0 : Numbers.LONG_NULL;
}

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: mtopolnik
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2025-11-10T14:28:48.329Z
Learning: In AsOfJoinDenseRecordCursorFactoryBase.java, the `backwardScanExhausted` flag is intentionally NOT reset in `toTop()` because backward scan results are reusable across cursor rewinds. The backward scan caches historical matches that remain valid when the cursor is rewound.

Learnt from: bluestreak01
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2025-11-07T00:59:31.522Z
Learning: In QuestDB's Cairo engine, transaction (_txn) files have a strong invariant: they are never truncated below TX_BASE_HEADER_SIZE. Once created, they are either fully formed (size >= header size) or completely removed along with the entire table directory when the table is dropped.