fix(sql): fix parquet read failure on chained window join by kafka1991 · Pull Request #6676 · questdb/questdb

kafka1991 · 2026-01-20T05:02:34Z

Fix parquet read failures in window chain join queries.

Root cause:

Chained window join should use GenericRecordMetadata.deepCopyOf() instead of copyOf()

coderabbitai · 2026-01-20T05:02:40Z

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.

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

Walkthrough

Changes introduce deep copying semantics for record metadata in join operations, update column count logic in Parquet frame processing to use indexed columns, and expand window join tests with Parquet conversion scenarios and updated test data expectations.

Changes

Cohort / File(s)	Summary
Metadata Deep Copying `core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java`, `core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java`	Added new `deepCopyOf()` method to GenericRecordMetadata with timestampIndex boundary guard; modified existing `copyOf()` to guard timestampIndex assignment. Updated SqlCodeGenerator join handling to use deep copies instead of shallow copies for intermediate window joins and non-keyed last-step AS OF/window joins.
Parquet Frame Processing `core/src/main/java/io/questdb/cairo/sql/PageFrameMemoryPool.java`	Changed column count computation in `openParquet()` from `addressCache.getColumnCount()` to `addressCache.getColumnIndexes().size()` for parquet column count validation and loop iteration.
Window Join Test Infrastructure `core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java`	Added `leftConvertParquet` and `rightConvertParquet` boolean flags with random initialization; introduced conditional Parquet conversion steps in test preparation. Updated NFLX timestamp test data expectations from 2023-01-01T09:19:00Z to 2023-01-02T09:19:00Z across multiple test outputs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Suggested labels

Bug, SQL

Suggested reviewers

bluestreak01
nwoolmer

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix(sql): fix parquet read failure on chained window join' accurately describes the main change—fixing a Parquet read issue in chained window joins.
Description check	✅ Passed	The description explains the root cause (using deepCopyOf instead of copyOf for chained window joins) and is directly related to the changes made in the pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

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

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

kafka1991 · 2026-01-20T15:05:09Z

@coderabbitai review

coderabbitai · 2026-01-20T15:05:18Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

bluestreak01 · 2026-01-22T22:33:24Z

@CodeRabbit can you review and report?

bluestreak01 · 2026-01-22T22:35:40Z

hey @kafka1991 here is AI spot:

  1. Naming Inconsistency in GenericRecordMetadata:

  The new deepCopyOf(RecordMetadata that, int columnCount) method name is misleading:

  - deepCopyOf(RecordMetadata that) (lines 115-136) truly deep-copies column metadata by creating new TableColumnMetadata objects
  - deepCopyOf(RecordMetadata that, int columnCount) (lines 103-113) does NOT deep-copy column metadata - it just skips the instanceof shortcut

  The actual difference is:
  // copyOf - may return same instance
  if (that instanceof GenericRecordMetadata) {
      return (GenericRecordMetadata) that;  // returns same instance!
  }

  // deepCopyOf - always creates new instance (but same column refs)
  // No instanceof check - always creates new GenericRecordMetadata

  The fix is correct - the bug was that copyOf returned the same instance which got modified. But a better name might be copyOfNew or forceCopyOf to avoid confusion with the true
  deep-copy overload.

coderabbitai · 2026-01-22T22:37:04Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@bluestreak01, I'll review the changes and provide a detailed report. Let me examine the code changes first.

🏁 Script executed:

#!/bin/bash
# Get the diff for all changed files in this PR
echo "=== GenericRecordMetadata.java changes ==="
git diff origin/master...HEAD -- core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java

echo -e "\n=== PageFrameMemoryPool.java changes ==="
git diff origin/master...HEAD -- core/src/main/java/io/questdb/cairo/sql/PageFrameMemoryPool.java

echo -e "\n=== SqlCodeGenerator.java changes ==="
git diff origin/master...HEAD -- core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java

echo -e "\n=== WindowJoinTest.java changes (first 200 lines) ==="
git diff origin/master...HEAD -- core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java | head -200

Length of output: 350

🏁 Script executed:

#!/bin/bash
# Fetch master branch and try again
git fetch origin master 2>/dev/null

# Try to get the diff using HEAD vs master
echo "=== GenericRecordMetadata.java changes ==="
git diff master -- core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java 2>/dev/null || \
git diff HEAD~1 -- core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java 2>/dev/null || \
cat core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java | grep -A 20 "deepCopyOf"

echo -e "\n=== PageFrameMemoryPool.java changes ==="
git diff master -- core/src/main/java/io/questdb/cairo/sql/PageFrameMemoryPool.java 2>/dev/null || \
git diff HEAD~1 -- core/src/main/java/io/questdb/cairo/sql/PageFrameMemoryPool.java 2>/dev/null || \
rg -A 5 -B 5 "readParquetColumnCount" core/src/main/java/io/questdb/cairo/sql/PageFrameMemoryPool.java

echo -e "\n=== SqlCodeGenerator.java changes ==="
git diff master -- core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java 2>/dev/null || \
git diff HEAD~1 -- core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java 2>/dev/null || \
rg -A 3 -B 3 "deepCopyOf" core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java

echo -e "\n=== Use GitHub CLI to get PR diff ==="
gh pr diff 6676 --repo questdb/questdb | head -300

Length of output: 20401

🏁 Script executed:

#!/bin/bash
# Let's understand the copyOfSansTimestamp implementation and the difference between copyOf and deepCopyOf
echo "=== Looking at copyOfSansTimestamp and related methods ==="
rg -A 20 "private static GenericRecordMetadata copyOfSansTimestamp" core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java

echo -e "\n=== Looking at existing deepCopyOf(RecordMetadata that) method ==="
rg -A 15 "public static GenericRecordMetadata deepCopyOf\(RecordMetadata that\)" core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java

echo -e "\n=== Check if there's a copyDenseOf or similar method ==="
rg "copyDenseOf|copyOf" core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java | head -20

Length of output: 1659

🏁 Script executed:

#!/bin/bash
# Get the full implementation of copyOfSansTimestamp methods
echo "=== Full copyOfSansTimestamp implementation ==="
cat core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java | grep -A 30 "copyOfSansTimestamp"

echo -e "\n=== Check the column metadata add method ==="
rg -A 5 "public void add\(TableColumnMetadata" core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java

echo -e "\n=== Check if GenericRecordMetadata is a subclass and what copyDenseOf does ==="
rg -B 3 "class GenericRecordMetadata" core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java

Length of output: 4060

🏁 Script executed:

#!/bin/bash
# Let's understand the copyColumns implementation to complete the picture
echo "=== Looking at copyColumns implementation ==="
rg -A 10 "private static void copyColumns" core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java

echo -e "\n=== Check test coverage - see if new methods are tested ==="
rg -n "deepCopyOf" core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java

echo -e "\n=== Check what issues the test is designed to catch ==="
rg -B 5 -A 5 "convertParquet|CONVERT PARTITION" core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java | head -40

Length of output: 713

🏁 Script executed:

#!/bin/bash
# Let's find the critical difference - check for the instanceof check in copyOf
echo "=== Checking the full copyOf method implementation ==="
cat core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java | sed -n '/public static GenericRecordMetadata copyOf(RecordMetadata that, int columnCount)/,/^    }/p'

echo -e "\n=== Comparing with deepCopyOf ==="
cat core/src/main/java/io/questdb/cairo/GenericRecordMetadata.java | sed -n '/public static GenericRecordMetadata deepCopyOf(RecordMetadata that, int columnCount)/,/^    }/p'

echo -e "\n=== Check if there are any unit tests for GenericRecordMetadata ==="
fd -t f "GenericRecordMetadata.*Test" core/src/test/
rg -l "GenericRecordMetadata" core/src/test/ | grep -i test | head -10

Length of output: 2126

🏁 Script executed:

#!/bin/bash
# Check if there are tests for the new deepCopyOf method
echo "=== Check GenericRecordMetadataTest for coverage of new method ==="
rg -A 30 "deepCopyOf" core/src/test/java/io/questdb/test/cairo/GenericRecordMetadataTest.java

echo -e "\n=== Check test file structure ==="
rg "public void test" core/src/test/java/io/questdb/test/cairo/GenericRecordMetadataTest.java | head -20

Length of output: 258

@bluestreak01, I've completed my review of the PR. Here's my detailed analysis:

📋 Review Report

✅ Summary

This PR successfully fixes Parquet read failures in chained window join queries. The root cause was identified correctly: shallow copying of metadata caused instance sharing issues in chained joins.

🔍 Key Changes Analysis

1. GenericRecordMetadata.java - Core Fix

Critical Discovery: The difference between copyOf() and deepCopyOf() is subtle but crucial:

copyOf(metadata, columnCount): Returns the SAME instance if input is already GenericRecordMetadata (optimization)
deepCopyOf(metadata, columnCount): ALWAYS creates a new copy, even for GenericRecordMetadata instances

This is the actual bug fix! In chained window joins, reusing the same metadata instance caused interference between joins, leading to Parquet read failures.

Additional Fix: Both methods now guard against invalid timestamp indexes:

if (timestampIndex < columnCount) {
    metadata.setTimestampIndex(timestampIndex);
}

This prevents out-of-bounds timestamp indexes when copying partial metadata.

2. SqlCodeGenerator.java - Application of Fix

Two strategic replacements:

Line 3765: Intermediate window join metadata handling
Line 3853: Last-step AS OF/window join metadata handling

Both locations are in chained join scenarios where metadata independence is critical.

3. PageFrameMemoryPool.java - Related Fix

Uses addressCache.getColumnIndexes().size() instead of getColumnCount() to accurately determine the number of columns to read from Parquet files. This aligns with the actual queried columns.

4. WindowJoinTest.java - Enhanced Test Coverage

Randomly converts tables to Parquet format during tests:

leftConvertParquet = rnd.nextBoolean();
rightConvertParquet = rnd.nextBoolean();

This ensures chained window joins are tested with Parquet data, catching similar issues in the future.

⚠️ Concerns

1. Low Test Coverage (60% for GenericRecordMetadata.java)

The new deepCopyOf(RecordMetadata, int) method lacks dedicated unit tests in GenericRecordMetadataTest.java. While integration tests in WindowJoinTest exercise this code path, direct unit tests would improve confidence and catch edge cases.

Recommendation: Add unit tests covering:

deepCopyOf() with various column counts
Timestamp index handling when timestampIndex >= columnCount
Verification that deepCopyOf always creates new instances (vs copyOf's optimization)

2. Semantic Naming Concern

The method deepCopyOf() creates the same depth of copy as copyOf() (both call copyOfSansTimestamp()). The real difference is whether instance reuse is allowed. Consider renaming to forceCopyOf() or documenting this behavior clearly.

🎯 Correctness Verification

The fix is sound because:

Chained window joins build upon previous join results
Each join potentially modifies metadata (adding columns, adjusting timestamp indexes)
Sharing metadata instances caused corruption when Parquet reading accessed column indexes
Forcing independent copies isolates each join's metadata modifications

📊 Overall Assessment

Verdict: ✅ Approve with minor recommendations

Fix Quality: Excellent - addresses root cause correctly
Test Coverage: Good - random Parquet conversion will catch regressions
Code Quality: Clean and focused
Documentation: Could be improved (see naming concern)

The changes are minimal, targeted, and solve the reported issue. The low unit test coverage is a minor concern but doesn't block approval given the strong integration test coverage.

Would you like me to trigger a full review, or do you have any questions about the implementation?

🧠 Learnings used

Learnt from: bluestreak01
Repo: questdb/questdb PR: 6673
File: core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java:2616-2618
Timestamp: 2026-01-19T21:43:19.228Z
Learning: In test `testWindowJoinFailsWhenSlaveDoesNotSupportTimeFrames` in `core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java`, the use of `AND` between timestamp equality conditions (e.g., `ts = 'X' AND ts = 'Y'`) is intentional to prevent intrinsic interval creation. This ensures the window join fails as expected when the slave doesn't support time frames, which is necessary after PR `#6673` added OR timestamp intrinsics recognition.

kafka1991 · 2026-01-23T00:58:16Z

hey @kafka1991 here is AI spot:

  1. Naming Inconsistency in GenericRecordMetadata:

  The new deepCopyOf(RecordMetadata that, int columnCount) method name is misleading:

  - deepCopyOf(RecordMetadata that) (lines 115-136) truly deep-copies column metadata by creating new TableColumnMetadata objects
  - deepCopyOf(RecordMetadata that, int columnCount) (lines 103-113) does NOT deep-copy column metadata - it just skips the instanceof shortcut

  The actual difference is:
  // copyOf - may return same instance
  if (that instanceof GenericRecordMetadata) {
      return (GenericRecordMetadata) that;  // returns same instance!
  }

  // deepCopyOf - always creates new instance (but same column refs)
  // No instanceof check - always creates new GenericRecordMetadata

  The fix is correct - the bug was that copyOf returned the same instance which got modified. But a better name might be copyOfNew or forceCopyOf to avoid confusion with the true
  deep-copy overload.

Thanks for the AI spot! I also reviewed other usages of deepCopy(RecordMetadata that) and there’s actually no need to deep-clone the columns in those cases. So I changed it to use a shallow copy of the columns instead.

glasstiger · 2026-01-23T01:24:59Z

[PR Coverage check]

😍 pass : 16 / 21 (76.19%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/cairo/GenericRecordMetadata.java	7	11	63.64%
🔵	io/questdb/cairo/sql/PageFrameMemoryPool.java	3	4	75.00%
🔵	io/questdb/griffin/SqlCodeGenerator.java	5	5	100.00%
🔵	io/questdb/griffin/LoopingRecordToRowCopier.java	1	1	100.00%

fix parquet read failed on window join

42d7f49

kafka1991 changed the title ~~fix(sql): fix parquet read failed on window join~~ fix(sql): fix parquet read failure on chained window join Jan 20, 2026

kafka1991 added 2 commits January 20, 2026 23:03

code review

5c1138e

Merge branch 'master' into window_join_parquet_fix

e4ee93e

Merge branch 'master' into window_join_parquet_fix

04b42f6

rename deepcopy to copyofnew

f400aa5

Merge branch 'master' into window_join_parquet_fix

6932741

bluestreak01 approved these changes Jan 23, 2026

View reviewed changes

bluestreak01 merged commit 6aa3dfb into master Jan 23, 2026
43 checks passed

bluestreak01 deleted the window_join_parquet_fix branch January 23, 2026 18:58

coderabbitai bot mentioned this pull request Mar 27, 2026

feat(sql): support lateral joins #6880

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sql): fix parquet read failure on chained window join#6676

fix(sql): fix parquet read failure on chained window join#6676
bluestreak01 merged 6 commits intomasterfrom
window_join_parquet_fix

kafka1991 commented Jan 20, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Jan 20, 2026 •

edited

Loading

Review skipped

Uh oh!

kafka1991 commented Jan 20, 2026

Uh oh!

coderabbitai bot commented Jan 20, 2026

Uh oh!

bluestreak01 commented Jan 22, 2026

Uh oh!

bluestreak01 commented Jan 22, 2026

Uh oh!

coderabbitai bot commented Jan 22, 2026

Uh oh!

kafka1991 commented Jan 23, 2026

Uh oh!

glasstiger commented Jan 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kafka1991 commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Estimated code review effort

Suggested labels

Suggested reviewers

Uh oh!

kafka1991 commented Jan 20, 2026

Uh oh!

coderabbitai bot commented Jan 20, 2026

Uh oh!

bluestreak01 commented Jan 22, 2026

Uh oh!

bluestreak01 commented Jan 22, 2026

Uh oh!

coderabbitai bot commented Jan 22, 2026

📋 Review Report

✅ Summary

🔍 Key Changes Analysis

1. GenericRecordMetadata.java - Core Fix

2. SqlCodeGenerator.java - Application of Fix

3. PageFrameMemoryPool.java - Related Fix

4. WindowJoinTest.java - Enhanced Test Coverage

⚠️ Concerns

1. Low Test Coverage (60% for GenericRecordMetadata.java)

2. Semantic Naming Concern

🎯 Correctness Verification

📊 Overall Assessment

Uh oh!

kafka1991 commented Jan 23, 2026

Uh oh!

glasstiger commented Jan 23, 2026

[PR Coverage check]

file detail

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kafka1991 commented Jan 20, 2026 •

edited

Loading

coderabbitai bot commented Jan 20, 2026 •

edited

Loading