Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@nwoolmer

Code Review

Critical

1. Test never actually injects stale metadata — passes vacuously
compat/src/test/java/io/questdb/compat/ParquetStaleMetadataTest.java:243-259

The 3-column stale metadata JSON is always longer than the 2-column original JSON:

• Original (2 cols): {"version":1,"schema":[{"column_type":775,"column_top":0},{"column_type":10,"column_top":0}]} (~93 chars)
• Stale (3 cols): {"version":1,"schema":[{"column_type":775,"column_top":0},{"column_type":26,"column_top":0},{"column_type":10,"column_top":0}]} (~127 chars)

The padding loop at line 249 only runs when staleMetaJson.length() < originalJson.length() (stale is shorter), which is the opposite of this case. The stale string is ~34 chars longer, so the loop body never executes. Then line 254 fires: staleMetaJson.length() > originalJson.length() is true, and the method returns at line 258 without injecting anything. The test then reads an unmodified parquet file, which read_parquet handles correctly regardless of the fix.

Additionally, if the HTTP /exp?fmt=parquet path does not embed QDB metadata (the BaseParquetExporter/HTTPSerialParquetExporter classes show no reference to QDB_META_KEY or QDB metadata), the first early-return at line 210-214 fires even before reaching the length comparison.

The test passes on CI today but never exercises the bug scenario. It would pass identically with or without the Rust fix.

Suggested fix: Reverse the injection direction — start with the 3-column file (which has longer QDB metadata) and replace it with a 2-column stale schema (shorter, so padding with spaces works). Or use QuestDB's PartitionEncoder directly to produce a file with controlled metadata, or write a Rust-level unit test in meta.rs that constructs a FileMetaData with a mismatched QDB schema.

2. Test assertions are too weak — only checks non-null
compat/src/test/java/io/questdb/compat/ParquetStaleMetadataTest.java:168

Assert.assertNotNull("read_parquet should return data", result);

Even if injection succeeded, this only verifies the HTTP response is non-null. It does not verify:

• Column count is 2 (not 3)
• Column types are correct (TIMESTAMP, DOUBLE — not TIMESTAMP, VARCHAR, DOUBLE)
• Actual data values

The entire point of the fix is that stale 3-column metadata causes Double data to be decoded as VARCHAR. The assertion must verify the schema and data are correct.

Moderate

3. Silent early returns should be test failures
compat/src/test/java/io/questdb/compat/ParquetStaleMetadataTest.java:213,257

Both early-return paths (qdbIdx < 0 at line 213, and metadata too long at line 257) log a message but silently return success. These should be Assert.fail("...") to ensure the test fails loudly if the metadata injection setup is broken, rather than producing false green CI.

4. Missing test for the reverse mismatch direction

The Rust fix discards QDB metadata whenever m.schema.len() != col_len. This covers both "more metadata columns than data columns" and "fewer metadata columns than data columns". Only the former is tested (attempted). The latter scenario (external tool adds columns to the Parquet data but preserves original narrower metadata) should also be tested.

5. No assertMemoryLeak() wrapper

The test allocates native memory through PartitionEncoder, PartitionDescriptor, TableReader, and Path. Per coding standards, tests that allocate native memory should use assertMemoryLeak(). This test would also be more robust as a unit test in ReadParquetFunctionTest where assertMemoryLeak() and assertQueryNoLeakCheck() are standard, and the binary-patching approach becomes unnecessary.

Minor

6. SQL statements use string concatenation instead of text blocks (""")

7. PR title is 71 characters (convention says under 70). Could shorten to fix(sql): fix read_parquet on Parquet files with stale QDB metadata (68 chars).

Production code verdict

The Rust fix itself is correct, safe, and minimal:

let qdb_meta = qdb_meta.filter(|m| m.schema.len() == col_len);

• All downstream code handles None gracefully, falling back to Parquet-native type inference
• No panic risk, no resource leak, no concurrency issue
• No false positives on valid QDB-written files (schema length always matches for QDB-authored Parquet)

Summary

Verdict: Request changes — the production fix is correct, but the test does not exercise it. The test needs to be rewritten to guarantee metadata injection succeeds and to assert on the actual query results (column count, types, values).

@bluestreak01 All findings addressed in f3e99a01 and b1415caa9:

Critical fixes:

1. Test no longer a no-op — flipped the injection direction. The test now exports a 3-column file (longer metadata), then replaces the 3-column QDB metadata with a shorter 2-column version padded to the same byte length. The injection succeeds reliably and the schema length mismatch (2 != 3) exercises the Rust fix.
2. Column type constant fixed — 775 replaced with ColumnType.TIMESTAMP | (1 << 17) = 131080.
3. Assertions strengthened — now verifies column names (ts, category, value), data value (1.5), row count ("count":10), and column count (exactly 3).
4. Silent early returns eliminated — all guard conditions use Assert.assertTrue / Assert.assertEquals.

Moderate fixes:
5. SQL uses multiline strings (""") per CLAUDE.md conventions.
6. PR title shortened to 68 chars (was 71).

Downgraded findings (false positives from initial review):

• "Missing assertMemoryLeak()" — compat tests extend AbstractTest (not AbstractCairoTest), which has no leak-checking infrastructure. No compat test uses assertMemoryLeak(). The test interacts via HTTP.
• "Discarding metadata loses LocalKeyIsGlobal for symbols" — the scenario only triggers on externally-rewritten files where symbols would have been re-encoded as plain strings.
• "Leaf vs field count could diverge for structs" — documented with a comment in meta.rs. No struct types exist today, and the fix degrades gracefully (falls back to physical type inference).

[PR Coverage check]

😍 pass : 1 / 1 (100.00%)

file detail