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

1511-1514: Pass partitionTxn in reloadColumnAt when reinitializing existing index readers

When an indexReader already exists for a column, reloadColumnAt reinitializes it without partitionTxn (uses the 5-arg default of(...), which sets partitionTxn = -1). For consistency and to support reloadConditionally decisions keyed on partitionTxn, pass the actual partition name txn.

Proposed change (outside this diff hunk; illustrative snippet):

// inside reloadColumnAt(...)
if (metadata.isColumnIndexed(columnIndex)) {
    BitmapIndexReader indexReader = indexReaders.getQuick(primaryIndex);
    if (indexReader != null) {
        final int partOffset = partitionIndex * PARTITIONS_SLOT_SIZE;
        final long partitionNameTxn = openPartitionInfo.getQuick(partOffset + PARTITIONS_SLOT_OFFSET_NAME_TXN);
        indexReader.of(configuration, path.trimTo(plen), name, columnTxn, partitionNameTxn, columnTop);
    }
}

This keeps reader.getPartitionTxn() accurate and avoids ambiguity in reloadConditionally().

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

107-156: Update call sites to new 6-arg AbstractIndexReader.of signature

Several call sites still invoke indexReader.of(…) with only five arguments. After changing the of(...) method to require an explicit partitionTxn before columnTop, these must be updated.

Please update the following locations to pass the missing partitionTxn (and retain the correct columnTop) in their calls:

• core/src/main/java/io/questdb/cairo/SymbolMapReaderImpl.java
  • Line 187
  • Line 232

• core/src/main/java/io/questdb/cairo/TableReader.java
  • Line 1513

Example diff:

- indexReader.of(config, path.trimTo(plen), columnName, columnNameTxn, columnTop);
+ indexReader.of(config, path.trimTo(plen), columnName, columnNameTxn, partitionTxn, columnTop);

Replace partitionTxn with the appropriate transaction value at each site.

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

57-63: Guard against potential overflow in maxValue + 1 when computing nulls

Minor: if maxValue can be Long.MAX_VALUE, maxValue + 1 overflows. Consider a safe add.

Example adjustment:

long hi = maxValue == Long.MAX_VALUE ? Long.MAX_VALUE : maxValue + 1;
nullCursor.nullCount = Math.min(columnTop, hi);

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

59-65: Same overflow nit for null prefix count

Minor: avoid potential overflow in maxValue + 1 when computing nullCount.

Example:

long hi = maxValue == Long.MAX_VALUE ? Long.MAX_VALUE : maxValue + 1;
nullCursor.nullCount = Math.min(columnTop, hi);

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

260-278: Reopen-or-reload flow is correct; minor readability nit on partition index

Logic to reuse an existing reader if present and reopen it with the correct partitionTxn is sound. For readability, pass the known partitionIndex directly instead of deriving it via getPartitionIndex(columnBase).

Apply within these lines:

-                    reader.of(
+                    reader.of(
                             configuration,
-                            pathGenNativePartition(getPartitionIndex(columnBase), partitionTxn),
+                            pathGenNativePartition(partitionIndex, partitionTxn),
                             metadata.getColumnName(columnIndex),
                             columnNameTxn,
                             partitionTxn,
                             getColumnTop(columnBase, columnIndex)
                     );

---

284-289: Helper accessor is fine; clarify semantics for callers

This returns the existing reader instance (which may be closed). Consider documenting that call sites should prefer getBitmapIndexReader(...) if they need an open/reloaded reader.

core/src/test/java/io/questdb/test/cairo/BitmapIndexConcurrentTest.java (6)

53-55: Fix misleading comment: only 100 distinct symbols are populated, not 1000.

Inner loop inserts SYM1..SYM100 once per partition. Total rows are 1000, distinct symbols are 100.

Apply this diff:

-            // Prepopulate with 10 partitions and 1000 distinct symbols
+            // Prepopulate with 10 partitions and 100 distinct symbols (10 occurrences per symbol initially)

---

61-67: Clarify population comment to match actual data shape.

The table is populated with 100 distinct symbols replicated across 10 partitions (not 1000 distinct symbols).

Apply this diff:

-        // Generate 1000 distinct symbols across 10 partitions using TableWriter API
+        // Generate 100 distinct symbols across 10 partitions (each symbol appears in every partition)

---

82-83: Shorten test duration or make it configurable to avoid slowing CI.

20 seconds per run is heavy for a unit/integration test and may impact CI stability.

Apply this diff to make it configurable with a sensible default:

-        final long testDurationMs = 20_000; // Run for 20 seconds
+        // Configurable test duration; default to 5s to keep CI fast.
+        final long testDurationMs = Long.getLong("qdb.test.bitmapIndexConcurrentMs", 5_000L);

---

129-131: Remove unreachable returns after Assert.fail.

Assert.fail throws AssertionError; subsequent returns are unreachable.

Apply this diff:

-                                        Assert.fail(lastError.get());
-                                        return;
+                                        Assert.fail(lastError.get());

-                                        Assert.fail(lastError.get());
-                                        return;
+                                        Assert.fail(lastError.get());

-                                    Assert.fail(lastError.get());
-                                    return;
+                                    Assert.fail(lastError.get());

-                    Assert.fail(lastError.get());
-                    return;
+                    Assert.fail(lastError.get());

Also applies to: 139-141, 147-149, 162-164

---

110-118: Optional: reduce SQL parsing/plan overhead by preparing once and binding a symbol.

Recompiling the query for each symbol adds overhead. If the test harness supports bind variables, prepare once:

• SQL: SELECT symbol, count(*) FROM trades WHERE symbol = $1 GROUP BY symbol
• Bind for each iteration.

If binding isn’t available in this harness, ignore.

Would you like me to provide a prepared-statement variant that works with AbstractCairoTest? If so, point me to the preferred compilation/binding helper in tests.

---

89-167: This test isn’t actually concurrent; consider adding a reader thread.

All inserts and queries run on the same thread sequentially. If you intend to stress concurrent writer/reader interaction, add:

• One thread continuously inserting/committing.
• Another thread continuously querying and validating.

This will better exercise index reader reloading across partition/column txns.

I can draft a two-threaded version with proper shutdown and exception propagation if you want.

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

40-40: Use the correct class in the logger to avoid misleading log source.

Currently logging is attributed to BitmapIndexBwdReader.

Apply this diff:

-    protected static final Log LOG = LogFactory.getLog(BitmapIndexBwdReader.class);
+    protected static final Log LOG = LogFactory.getLog(AbstractIndexReader.class);

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

49-52: API extension is sound; default overload preserves source compatibility.

• Adding getColumnTxn/getPartitionTxn provides necessary visibility for reader lifecycle decisions.
• The 5-arg default of(...) cleanly forwards to the 6-arg form with partitionTxn = -1.

Consider documenting sentinel semantics in Javadoc:

• partitionTxn = -1 denotes “unknown/not tracked”
• getColumnTxn/getPartitionTxn return values must reflect the last of(...) call (even for null readers).

Also applies to: 76-77, 86-104

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro