fix(sql): fix wrong results from queries on renamed Parquet columns

[bluestreak01]

Fixes #7316

Summary

Equality and IN filters on a renamed column of a Parquet partition could
return too few rows (often zero), silently dropping valid data. The defect
is in Parquet row-group bloom-filter pushdown, not in any index or in the
bloom filter itself.

The pushdown resolves the filtered column against the Parquet file's column
names. Those names are frozen when the partition is converted to Parquet, so
a later RENAME COLUMN leaves them stale. When some other column already
carries the query's current name (common after a chain of renames), the
pushdown looks up that name, lands on the wrong Parquet column, checks that
column's bloom filter, gets a false negative, and skips the whole row group
before any row is scanned. The rows are never returned.

The normal data-decode path was never affected: it maps columns by stable
column id (getColumnId, which equals the table writer index), so it always
reads the correct column regardless of renames. Only the pruning path used
name resolution, which is why a plain col::string = 'x' (cast suppresses
pushdown) returned the rows while col = 'x' did not.

Root cause walkthrough

Reproduced from WalWriterFuzzTest.testCreateTableAsParquet
(seeds 1504463487934207962L, 2622578810447144450L). The fuzz table renamed
c2 -> new_col_5 while a different column had earlier been named new_col_5.
In the Parquet metadata:

• index 0: c2 (id=1) -- the actual symbol column holding 'YV'
• index 10: new_col_5 (id=15) -- an unrelated column

The query WHERE "new_col_5" = 'YV' resolved the name new_col_5 to Parquet
index 10 and tested that column's bloom filter, which does not contain 'YV',
so the row group was pruned and the query returned 0 rows. The reference
non-WAL table returned the 6 matching rows.

Fix

Make the native-table pushdown resolve columns by stable id, the same way the
data-decode path already does:

• PushdownFilterExtractor.PushdownFilterCondition carries the column's stable
  writer index (column id).
• ParquetMetaFileReader gains getColumnIndexById(), mirroring
  PageFrameMemoryPool.buildColumnIdMap's id < 0 ? positional fallback for
  external files without field ids. Native-table partitions reach the pruning
  path through ParquetPartitionDecoder.metadata(), which returns a
  ParquetMetaFileReader, so this is the only resolver that runs by id.
• ParquetRowGroupFilter.prepareFilterList takes a resolveByColumnId flag.
  Native-table cursors (FwdTableReaderPageFrameCursor,
  BwdTableReaderPageFrameCursor) pass true and resolve by id.
  read_parquet() cursors pass false and keep name resolution, matching how
  read_parquet() already projects external files by name in
  canProjectMetadata.

Cleanup

prepareFilterListImpl previously carried a by-id branch on
ParquetFileDecoder.Metadata as well, but no caller ever reached it: the only
callers of the Metadata overload are the two read_parquet() cursors, and
both pass resolveByColumnId=false. The commit drops the unreachable
ParquetFileDecoder.Metadata.getColumnIndexById() overload and collapses the
dead ternary into a direct ParquetMetaFileReader call, guarded by an assert
that documents the invariant.

Effects and tradeoffs

• Correctness: filters on renamed columns of Parquet partitions now match the
  same rows as native partitions. This is the data-loss fix.
• read_parquet() behaviour is unchanged: it keeps name-based resolution, which
  is the right semantics for arbitrary external files (no QuestDB column ids,
  columns matched by name).
• Pruning effectiveness is unchanged for the common (non-renamed) case: when the
  name and id point at the same column the result is identical; only the lookup
  key changed. Row groups are still pruned for genuinely absent values (covered
  by tests that assert the prune still fires).
• Negligible cost: getColumnIndexById is a linear scan over the partition's
  column list, run once per partition during filter-list preparation, the same
  shape as the existing getColumnIndex(name) scan it replaces. No extra work
  on the per-row or per-row-group path.
• Scope limited to the row-group pruning column resolution; the bloom filter
  format, statistics, and decode paths are untouched.

Test plan

• ParquetRowGroupPruningTest#testBloomFilterSymbolRenamedColumn renames a
  symbol column into a name previously held by another column, then asserts
  equality on the bloom-filter path returns the right rows and that an absent
  value still prunes.
• Six additional rename-scenario tests cover the min/max-statistics path, which
  shares the same column resolution: testRenamedColumnMinMaxEquality,
  testRenamedColumnRange, testRenamedColumnBetween, testRenamedColumnInList,
  testRenamedColumnOrEquality, and testRenamedColumnIsNull (the last covering
  the null-count path for IS [NOT] NULL). Each swaps two columns' names so the
  stale parquet name targets the wrong column's statistics, then asserts matching
  rows survive and genuinely-absent values still prune.
• Verified that all six new tests, plus the existing bloom test, fail when the
  by-id resolution is reverted to by-name, and pass with the fix.
• WalWriterFuzzTest#testCreateTableAsParquet passes on the reported seeds and
  across several fresh random-seed runs.
• ParquetRowGroupPruningTest (127), ReadParquetFunctionTest (110),
  ParquetTest, sqllogic parquet.ParquetTest (46), and
  AlterTableRenameColumnTest (30) all pass.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1230179c-dacc-46c5-9d0f-625e23378e26

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-parquet-pushdown-renamed-column

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[bluestreak01]

Review (level 3, mission-critical pass)

Full pass: change-surface map, all agent dimensions (correctness, concurrency, performance/optimality, resources, tests, code quality, metadata, cross-context, adversarial), and per-finding source verification. No Rust files in the diff.

Change summary (verified)

The bug: native-table Parquet row-group pruning resolved the filtered column against the frozen Parquet column name. After RENAME COLUMN that name is stale; when another column already carries the query's current name, pruning checked the wrong column's bloom filter / stats and dropped the whole row group, causing silent data loss. The decode path was unaffected because it resolves by stable column id (writerIndex).

The fix threads the column's stable writerIndex into PushdownFilterCondition and adds getColumnIndexById() so native-table pruning resolves by id (resolveByColumnId=true), while read_parquet() keeps name resolution (false). The resolution chain checks out end-to-end:

• PartitionEncoder.populateFromTableReader writes columnId = getWriterIndex() into the _pm metadata at conversion (frozen, never changes on rename).
• Native pruning now calls ParquetMetaFileReader.getColumnIndexById(writerIndex), reading the same parquetMetaReader.getColumnId(i) that the decode path's PageFrameMemoryPool.buildColumnIdMap already consumes, keyed by the same metadata.getWriterIndex(). Pruning resolution == decode resolution.

The fix is also strictly broader than the reported symptom: it additionally repairs the plain-rename case (old code silently lost pruning) and the drop-then-re-add-same-name case (old code could wrongly prune).

Critical

None.

Moderate

None.

Minor

1. Dead code: ParquetFileDecoder.Metadata.getColumnIndexById is currently unreachable (ParquetFileDecoder.java:484). The only callers of the ParquetFileDecoder.Metadata overload of prepareFilterList are the two read_parquet() cursors, and both pass resolveByColumnId=false, so they always take the getColumnIndex(name) branch. The active fix path uses ParquetMetaFileReader.getColumnIndexById (native tables go through ParquetPartitionDecoder.metadata(), which returns ParquetMetaFileReader). The Metadata version is defensible as symmetric API but is not exercised — consider a short comment noting it's there for symmetry, or drop it. Non-blocking.

2. Test coverage is narrow relative to the fix's reach (ParquetRowGroupPruningTest.java:817). testBloomFilterSymbolRenamedColumn exercises only SYMBOL equality (bloom path). The same resolution change governs min/max-stats range filters, IN, OR-of-equalities, BETWEEN, and IS [NOT] NULL on renamed columns — none are tested for the rename scenario. They all funnel through the one prepareFilterList resolution, so the single test does cover the resolution fix, but a range/min-max test on a renamed numeric column would guard the broader contract cheaply. The test itself is well-formed: .returns(...) (not returnsOnce), the assertMemoryLeak + .noLeakCheck() pairing is legitimate (the lambda holds many statements sharing one leak scope), and the prune-still-fires assertion (getRowGroupsSkipped() > 0) is correctly reset beforehand and robust against .returns()'s double pass.

Downgraded (false positives, verified and dismissed)

• "Constructor signature change breaks callers" — all 7 new PushdownFilterCondition(...) sites are in PushdownFilterExtractor and updated; a repo-wide grep (incl. tests/benchmarks) found none elsewhere.
• "writerIndex is lost through GenericRecordMetadata.copyOfNew" — copyColumns shallow-copies the TableColumnMetadata references, so getWriterIndex() returns the original stable id at all three extractAndCompile callsites.
• "Pruning resolution may diverge from the decode path" — both read the same parquetMetaReader.getColumnId(i) keyed by the same getWriterIndex(); results are identical (ids are unique for native tables, so the hashmap-last-wins vs linear-scan-first-wins distinction is moot).
• "getWriterIndex throws / is invalid for read_parquet metadata" — returns -1 (those TableColumnMetadata have writerIndex=-1); harmless because read_parquet passes false and never reads it.
• "getColumnIndexById linear scan is a perf regression" — identical O(columns) shape to the getColumnIndex(name) scan it replaces, run once per partition; the decode path's O(1) map isn't warranted for a handful of conditions.
• "id < 0 positional fallback could mismatch for native tables" — native conversion always writes writerIndex >= 0; the fallback only matters for external read_parquet files, which resolve by name anyway.
• Concurrency — the new field is a final int set at compile time and read-only thereafter; no new shared mutable state.

Summary

• Verdict: approve. The fix is correct, minimal, and well-targeted; it aligns the pruning column-resolution with the already-correct decode path, and the description's effects/tradeoffs are accurate.
• No regressions or tradeoffs identified: pruning effectiveness is unchanged for non-renamed columns, read_parquet() semantics are preserved, and the cost is the same per-partition scan as before.
• Draft findings: ~9 considered, 2 verified (both Minor), 7 dropped as false positives after source verification.
• In-diff vs out-of-diff: 2 in-diff, 0 out-of-diff. The zero out-of-diff count is justified — every consumer of the changed contract was traced (decode path in PageFrameMemoryPool, conversion in PartitionEncoder, metadata copy in GenericRecordMetadata, and all three SqlCodeGenerator callsites) and each upholds the new id-based resolution by construction.

_{Generated with a level-3 automated review pass.}

[jovfer]

LGTM, if we OK to ignore dead code (minor 1 in the review above)

[bluestreak01]

@jovfer thanks, dead code was removed

[bluestreak01]

/azp run macwin

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mtopolnik]

[PR Coverage check]

😍 pass : 22 / 24 (91.67%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/cairo/ParquetMetaFileReader.java	4	5	80.00%
🔵	io/questdb/griffin/engine/table/ParquetRowGroupFilter.java	7	8	87.50%
🔵	io/questdb/griffin/engine/table/PushdownFilterExtractor.java	11	11	100.00%

[questdb-butler]

⚠️ Enterprise CI Failed

The enterprise test suite failed for this PR.

Build: View Details
Tested Commit: 6f6f0b4f0488a1f8b0f78e3bdfa456696528ac57

Please investigate the failure before merging.