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

196-196: ⚠️ Potential issue | 🟠 Major

Avoid per-call free/realloc of bloomFilterIndexes; keep one lifecycle and free in doClose().

At Line 1611, Misc.free(bloomFilterIndexes) inside the conversion path forces repetitive native free/alloc cycles across partition conversions, and it also leaves cleanup ambiguous when conversion is never invoked. Reuse the list per call (reopen + clear) and release it once in doClose().

♻️ Suggested fix

@@
-                try {
-                    if (bloomFilterColumns != null && !bloomFilterColumns.isEmpty()) {
-                        bloomFilterIndexes.reopen();
-                        parseBloomFilterColumnIndexes(bloomFilterColumns, bloomFilterIndexes);
-                        bloomFilterColumnIndexesPtr = bloomFilterIndexes.getAddress();
-                        bloomFilterColumnCount = (int) bloomFilterIndexes.size();
-                    }
+                if (bloomFilterColumns != null && !bloomFilterColumns.isEmpty()) {
+                    bloomFilterIndexes.reopen();
+                    bloomFilterIndexes.clear();
+                    parseBloomFilterColumnIndexes(bloomFilterColumns, bloomFilterIndexes);
+                    bloomFilterColumnIndexesPtr = bloomFilterIndexes.getAddress();
+                    bloomFilterColumnCount = (int) bloomFilterIndexes.size();
+                }
 
-                    PartitionEncoder.encodeWithOptions(
-                            partitionDescriptor,
-                            other,
-                            ParquetCompression.packCompressionCodecLevel(compressionCodec, compressionLevel),
-                            statisticsEnabled,
-                            rawArrayEncoding,
-                            rowGroupSize,
-                            dataPageSize,
-                            parquetVersion,
-                            bloomFilterColumnIndexesPtr,
-                            bloomFilterColumnCount,
-                            fpp
-                    );
-                } finally {
-                    Misc.free(bloomFilterIndexes);
-                }
+                PartitionEncoder.encodeWithOptions(
+                        partitionDescriptor,
+                        other,
+                        ParquetCompression.packCompressionCodecLevel(compressionCodec, compressionLevel),
+                        statisticsEnabled,
+                        rawArrayEncoding,
+                        rowGroupSize,
+                        dataPageSize,
+                        parquetVersion,
+                        bloomFilterColumnIndexesPtr,
+                        bloomFilterColumnCount,
+                        fpp
+                );

// In doClose():
Misc.free(bloomFilterIndexes);

Also applies to: 1589-1612

🤖 Prompt for AI Agents

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

In `@core/src/main/java/io/questdb/cairo/TableWriter.java` at line 196, The field
bloomFilterIndexes in TableWriter is being freed per conversion call causing
repeated native free/alloc; instead keep it allocated for the TableWriter
lifecycle, clear and reuse it during conversions (use reopen/clear on
bloomFilterIndexes where conversion logic runs) and move the
Misc.free(bloomFilterIndexes) call into TableWriter.doClose() so it is released
once when the writer is closed; update conversion code to remove per-call
Misc.free and replace with bloomFilterIndexes.clear() or equivalent reuse logic.

---

1587-1587: ⚠️ Potential issue | 🟠 Major

Validate fpp before passing it into native encoding.

Line 1587 accepts infinities and out-of-range values; these should be rejected early to avoid undefined native behavior.

🛡️ Suggested fix

                 double fpp = Double.isNaN(bloomFilterFpp) ? config.getPartitionEncoderParquetBloomFilterFpp() : bloomFilterFpp;
+                if (!Double.isFinite(fpp) || fpp <= 0.0 || fpp >= 1.0) {
+                    throw CairoException.nonCritical()
+                            .put("bloom_filter_fpp must be in (0,1), got: ")
+                            .put(fpp);
+                }

🤖 Prompt for AI Agents

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

In `@core/src/main/java/io/questdb/cairo/TableWriter.java` at line 1587, Validate
the computed fpp in TableWriter (the local variable fpp computed from
bloomFilterFpp or config.getPartitionEncoderParquetBloomFilterFpp()) before
passing it into native encoding: ensure it's finite (not NaN or infinite) and
within the valid probability range (0.0 < fpp < 1.0, or use your project's
accepted bounds), and if not throw an IllegalArgumentException with a clear
message; update the code path that calls the native encoder to use this
validated fpp so invalid values are rejected early.

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

77-79: ⚠️ Potential issue | 🟡 Minor

Rename boolean locals to is.../has... prefixes.

Line 77 (skip), Line 145 (supported), and Line 212 (allCompatible) should follow the repository boolean naming rule.

As per coding guidelines **/*.java: “When choosing a name for a boolean variable, field or method, always use the is... or has... prefix, as appropriate”.

Also applies to: 145-146, 212-221

🤖 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/ParquetRowGroupFilter.java`
around lines 77 - 79, Rename the boolean local variables in
ParquetRowGroupFilter to follow the is.../has... convention: change skip (used
with decoder.canSkipRowGroup) to isSkippableRowGroup or isSkippable, change
supported to isSupported, and change allCompatible to areAllCompatible (or
isAllCompatible if you prefer singular). Update every use site within the
method(s) (including the conditional checks and any increments like
rowGroupsSkipped.incrementAndGet()) to reference the new names so compilation
and logic remain unchanged.

core/rust/qdbr/src/parquet_write/file.rs (1)

158-164: ⚠️ Potential issue | 🟠 Major

Use runtime FPP validation in release builds, not only debug_assert!.

At Line 159 this check disappears in release builds, so invalid fpp can still reach bloom sizing math. Please enforce 0.0 < fpp < 1.0 at runtime (assert!/error return) in the public setter.

#!/bin/bash
# Verify whether FPP validation in Rust is debug-only vs runtime-enforced.
rg -n "with_bloom_filter_fpp|debug_assert!|assert!|bloom_filter_fpp\\s*[<>]" \
  core/rust/qdbr/src/parquet_write/file.rs \
  core/rust/qdbr/src/parquet_write/jni.rs

Expected result: if with_bloom_filter_fpp only has debug_assert!, runtime validation is not guaranteed in release.

🤖 Prompt for AI Agents

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

In `@core/rust/qdbr/src/parquet_write/file.rs` around lines 158 - 164, The
debug-only check in with_bloom_filter_fpp allows invalid fpp in release builds;
replace the debug_assert! with runtime validation: either use assert!(fpp > 0.0
&& fpp < 1.0, "...") to panic on invalid input at runtime or (preferable for a
public setter) change with_bloom_filter_fpp signature to return Result<Self,
Error> and return an Err when fpp is out of range, then only set
self.bloom_filter_fpp = fpp and return Ok(self) when valid; update callers
accordingly. Ensure you reference the with_bloom_filter_fpp method and the
bloom_filter_fpp field when making the change.

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

115-179: ⚠️ Potential issue | 🟠 Major

Bloom-option positive-path tests still don’t prove bloom options were applied.

Line 126, Line 142, Line 158, and Line 175 currently only prove conversion succeeded (assertPartitionExists). These would still pass if bloom_filter_columns / fpp were parsed but ignored in the convert path.

Suggested follow-up assertion pattern

+import io.questdb.griffin.engine.table.ParquetRowGroupFilter;
...
 execute("ALTER TABLE " + tableName + " CONVERT PARTITION TO PARQUET LIST '2024-06-10' WITH (bloom_filter_columns = 'id')");
 assertPartitionExists(tableName, "2024-06-10.3");
+ParquetRowGroupFilter.resetRowGroupsSkipped();
+assertQueryNoLeakCheck(
+        "cnt\n0\n",
+        "SELECT COUNT(*) cnt FROM " + tableName + " WHERE id = -1",
+        null,
+        false,
+        true
+);
+Assert.assertTrue(ParquetRowGroupFilter.getRowGroupsSkipped() > 0);

🤖 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/AlterTableConvertPartitionTest.java`
around lines 115 - 179, Tests like
testConvertAllPartitionsWithBloomFilterColumns /
testConvertAllPartitionsWithBloomFilterAndFpp /
testConvertAllPartitionsWithBloomFilterFpp /
testConvertAllPartitionsWithBloomFilterWhere only assert conversion succeeded;
update each test to also verify that bloom options were applied by inspecting
the converted partition output after execute(...) and
assertPartitionExists(...). Concretely: locate the created partition (use the
same tableName and partition name used in assertPartitionExists), read the
partition’s Parquet/metadata or bloom artifacts (e.g., parquet file footer
metadata keys for bloom_filter_columns and fpp or presence/content of bloom
filter files for column "id"), and add assertions that the bloom_filter_columns
contains "id" and that fpp matches the provided value (e.g., "0.05", "0.01",
"0.1"). Ensure these checks run in each corresponding test immediately after
assertPartitionExists so the tests fail if the options are parsed but not
applied.

core/src/main/java/io/questdb/cutlass/parquet/CopyExportRequestTask.java (1)

589-619: ⚠️ Potential issue | 🟠 Major

Validate bloom filter FPP at the last pre-native boundary.

fpp is passed into createStreamingParquetWriter(...) without any local range/finite validation (Line 591-618). Even if some call sites validate, this is the final JVM boundary before JNI/native allocation logic, so it’s worth failing fast here too.

This was raised earlier; re-flagging because it still appears unvalidated in the current snippet.

Proposed fix (finite + (0,1) check)

diff --git a/core/src/main/java/io/questdb/cutlass/parquet/CopyExportRequestTask.java b/core/src/main/java/io/questdb/cutlass/parquet/CopyExportRequestTask.java
@@
             long bloomFilterIndexesPtr = 0;
             int bloomFilterCount = 0;
             double fpp = Double.isNaN(bloomFilterFpp) ? DEFAULT_BLOOM_FILTER_FPP : bloomFilterFpp;
+            if (!Double.isFinite(fpp) || fpp <= 0.0d || fpp >= 1.0d) {
+                throw CairoException.nonCritical()
+                        .put("bloom_filter_fpp must be between 0 and 1 (exclusive): ")
+                        .put(fpp);
+            }
 
             if (bloomFilterColumns != null && !bloomFilterColumns.isEmpty()) {
                 bloomFilterColumnIndexes.reopen();
                 parseBloomFilterColumnIndexes(bloomFilterColumns, metadata, bloomFilterColumnIndexes, bloomFilterColumnsPosition);
                 if (bloomFilterColumnIndexes.size() > 0) {
                     bloomFilterIndexesPtr = bloomFilterColumnIndexes.getAddress();
                     bloomFilterCount = (int) bloomFilterColumnIndexes.size();
                 }
             }

🤖 Prompt for AI Agents

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

In `@core/src/main/java/io/questdb/cutlass/parquet/CopyExportRequestTask.java`
around lines 589 - 619, The code uses fpp (derived from bloomFilterFpp) and
passes it to createStreamingParquetWriter without validating it's a finite
probability in (0,1); update CopyExportRequestTask to validate fpp after
computing it (the local variable fpp) to ensure Double.isFinite(fpp) and fpp >
0.0 && fpp < 1.0, and if not throw an IllegalArgumentException (or similar) with
a clear message including the invalid value and parameter name before calling
createStreamingParquetWriter; this protects the native boundary that expects a
valid false-positive probability.

core/rust/qdbr/src/parquet_write/util.rs (1)

428-442: Tighten the boundary test wording and final assertion.

Line 428’s comment is slightly misleading (i32::MIN is not the largest u32), and adding one final max assertion would better lock the invariant.

Suggested test refinement

-        // i32::MIN (0x80000000) is largest as unsigned (2^31)
+        // i32::MIN (0x80000000) is larger than i32::MAX under u32 ordering (2^31)
         // i32::MAX (0x7FFFFFFF) is 2^31 - 1 as unsigned
@@
         mm.update_unsigned(i32::MIN); // 0x80000000 = 2147483648u32
         assert_eq!(mm.min, Some(0)); // 0 is still the smallest unsigned
+        assert_eq!(mm.max, Some(-1)); // 0xFFFFFFFF remains the largest unsigned

🤖 Prompt for AI Agents

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

In `@core/rust/qdbr/src/parquet_write/util.rs` around lines 428 - 442, The
boundary test comments and final assertion around mm.update_unsigned are
misleading: update the comment for the i32::MIN case to state that i32::MIN maps
to 0x80000000 (2147483648u32) rather than calling it the largest u32, and add
one more assertion after the i32::MIN update to assert mm.max is still Some(-1)
so the invariant is fully checked; locate the test block using
mm.update_unsigned, mm.min and mm.max and adjust the comment text and append the
final max assertion accordingly.

core/src/test/java/io/questdb/test/std/str/CharsTest.java (1)

386-422: Good test coverage for the code point ordering change.

The tests comprehensively cover null handling, equality, basic ordering, prefix comparisons, empty strings, and supplementary characters. The surrogate pair test case (\uD834\uDD1E > "z") correctly validates that code point ordering is used rather than UTF-16 code unit ordering.

Consider adding a test case that would fail under the old per-char comparison but passes under code point comparison, to document the behavioral change:

💡 Optional: Add test for BMP vs supplementary ordering

// U+FFFF (BMP) vs U+10000 (first supplementary)
// Under char comparison: '\uFFFF' (65535) > '\uD800' (55296) → wrong
// Under code point comparison: 65535 < 65536 → correct
Assert.assertTrue(Chars.lessThan("\uFFFF", "\uD800\uDC00"));
Assert.assertTrue(Chars.greaterThan("\uD800\uDC00", "\uFFFF"));

🤖 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/std/str/CharsTest.java` around lines 386 -
422, Add an explicit BMP vs supplementary comparison to
testGreaterThanAndLessThan that would fail under per-char ordering but pass
under code point ordering: in the CharsTest.testGreaterThanAndLessThan method,
add assertions using Chars.lessThan("\uFFFF", "\uD800\uDC00") and
Chars.greaterThan("\uD800\uDC00", "\uFFFF") (i.e., compare U+FFFF to the
surrogate pair for U+10000) to document and verify the intended code point
semantics.

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

7114-7154: Optional perf refactor: precompute descriptor indexes once in parseBloomFilterColumnIndexes.

The current implementation recomputes descriptor positions by scanning prior columns for each token (Line 7132 loop), which is avoidable overhead on wide schemas / large column lists. Consider building a one-pass metadataIndex→descriptorIndex map before token parsing.

🤖 Prompt for AI Agents

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

In `@core/src/main/java/io/questdb/cairo/TableWriter.java` around lines 7114 -
7154, The parseBloomFilterColumnIndexes method is recomputing descriptorIndex by
scanning prior columns for every token; precompute a
metadataIndex→descriptorIndex mapping once and use it during token parsing to
avoid the inner loop. Add a small int[] or DirectIntList (size
metadata.getColumnCount()) and populate it in a single pass over metadata
(incrementing a counter when metadata.getColumnType(i) > 0) to record
descriptorIndex for each metadata index, then replace the per-token loop that
computes descriptorIndex with a constant-time lookup into that map inside
parseBloomFilterColumnIndexes (keep existing checks for metadataIndex >= 0 and
duplicate detection).

core/rust/qdbr/src/parquet_write/file.rs (2)

37-55: WriteOptions cloning is now on the hot path after adding HashSet.

Line 850 clones WriteOptions per page; after Line 52 this deep-clones bloom_filter_columns repeatedly. Consider making that field cheap-to-clone (e.g., Arc<HashSet<usize>>) or keeping bloom-column selection out of per-page options.

💡 Minimal direction to reduce clone cost

- pub bloom_filter_columns: HashSet<usize>,
+ pub bloom_filter_columns: Arc<HashSet<usize>>,

- pub fn with_bloom_filter_columns(mut self, columns: HashSet<usize>) -> Self {
-     self.bloom_filter_columns = columns;
+ pub fn with_bloom_filter_columns(mut self, columns: HashSet<usize>) -> Self {
+     self.bloom_filter_columns = Arc::new(columns);
      self
  }

Also applies to: 841-853

🤖 Prompt for AI Agents

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

In `@core/rust/qdbr/src/parquet_write/file.rs` around lines 37 - 55, WriteOptions
is being cloned on the hot path and its bloom_filter_columns (HashSet<usize>)
causes expensive deep clones; change bloom_filter_columns to a cheap-to-clone
type (e.g., Arc<HashSet<usize>>) or remove it from per-page WriteOptions and
pass bloom column selection separately to the per-page writer. Update the
WriteOptions struct (symbol: WriteOptions) to use Arc<HashSet<usize>> for
bloom_filter_columns and adjust all usages/constructors and clones (including
where WriteOptions is cloned around line ~850) to account for Arc, or refactor
the code that calls WriteOptions.clone() to accept a separate
&HashSet/Arc<HashSet> parameter instead.

---

530-551: Avoid eager page materialization across partitions to cap peak memory.

collect_multi_partition_pages builds a full Vec of pages before compression. For large multi-partition row groups this can inflate RSS and add latency. Prefer streaming/flattening iterators directly into Compressor.

🤖 Prompt for AI Agents

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

In `@core/rust/qdbr/src/parquet_write/file.rs` around lines 530 - 551,
collect_multi_partition_pages currently eagerly materializes all Page values
into the all_pages Vec causing high memory use; change it to stream pages
instead by removing the Vec allocation and either (A) change the function to
return a streaming iterator (e.g., impl Iterator<Item = ParquetResult<Page>> or
a boxed Box<dyn Iterator...>) that yields the results of chaining/flattening
each column_chunk_to_pages(...) iterator (use Iterator::chain or flat_map), or
(B) change the API to accept a sink/Compressor callback and directly feed each
pages_iter into the Compressor as you iterate. Update callers to consume the
iterator or pass the compressor so pages are processed incrementally rather than
collected into all_pages. Ensure references to collect_multi_partition_pages,
column_chunk_to_pages, and Compressor are updated accordingly.

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

103-107: Boolean identifiers should follow the is... / has... convention.

allConstant and containsQuery(...) don’t follow the project boolean naming rule.

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-152

🤖 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/PushdownFilterExtractor.java`
around lines 103 - 107, Rename boolean identifiers to follow the is/has
convention: change the local variable allConstant to a boolean name like
areAllConstants (or isAllConstant) and rename the method containsQuery(...) to
hasQuery(...). Update all references inside PushdownFilterExtractor (search for
allConstant and containsQuery), including the related occurrences around lines
136-152, plus any call sites, imports, and tests to use the new names so
compilation succeeds.

core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java (1)

3188-3196: Boolean local names in this block should follow is.../has... convention.

enableParallelFilter and enablePreTouch should be renamed to match repo naming rules for booleans.

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/main/java/io/questdb/griffin/SqlCodeGenerator.java` around lines
3188 - 3196, Rename the two local boolean variables to follow the is/has
convention: change enableParallelFilter to isParallelFilterEnabled and
enablePreTouch to isPreTouchEnabled (assignments remain the same:
isParallelFilterEnabled = executionContext.isParallelFilterEnabled();
isPreTouchEnabled = SqlHints.hasEnablePreTouchHint(model, model.getName());),
and update all subsequent references in this block (including the if check that
uses factory.supportsPageFrameCursor() and the later logic that uses these
booleans alongside useJit and canCompile) to use the new names.

core/src/main/java/io/questdb/cutlass/parquet/CopyExportRequestTask.java (1)

85-113: De-duplicate bloom-filter column tokenization + keep static helpers grouped.

validateBloomFilterColumns() and parseBloomFilterColumnIndexes() implement the same comma-splitting + trimming loop. It’s easy for these to drift again (e.g., one starts accepting whitespace-only tokens, the other rejects). Recommend extracting a shared private static tokenizer/helper so both callers share the exact semantics while still throwing SqlException vs CairoException.

Also, since both helpers are static, consider co-locating them (and keeping static members sorted) to match the repo’s member ordering rules. As per coding guidelines, **/*.java: “Group Java class members by kind (static vs. instance) and visibility, sorted alphabetically.”

Also applies to: 347-375

🤖 Prompt for AI Agents

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

In `@core/src/main/java/io/questdb/cutlass/parquet/CopyExportRequestTask.java`
around lines 85 - 113, Extract the comma-splitting + trimming logic used in
validateBloomFilterColumns and parseBloomFilterColumnIndexes into a single
private static helper (e.g., tokenizeColumnList or splitAndTrimColumns) that
returns an iterable/list of CharSequence tokens and accepts the original
CharSequence input and a base position; then update validateBloomFilterColumns
to iterate that helper's tokens and perform meta.getColumnIndexQuiet checks,
throwing SqlException with the appropriate position, and update
parseBloomFilterColumnIndexes to reuse the same helper but translate
missing-column cases into CairoException as before; finally, move and group
these new static helper(s) alongside other static members and sort
alphabetically to satisfy the class member ordering rules.