Skip to content

GH-3267: Add comprehensive assertions to TestMemPageStore #3268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wgtmac merged 2 commits into from
Sep 2, 2025
Merged

GH-3267: Add comprehensive assertions to TestMemPageStore #3268

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26ce9e8
Update
ArnavBalyan Aug 15, 2025
2389db8
update
ArnavBalyan Aug 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions parquet-column/src/test/java/org/apache/parquet/column/mem/TestMemPageStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,15 @@

import static org.apache.parquet.column.Encoding.BIT_PACKED;
import static org.apache.parquet.column.Encoding.PLAIN;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

import java.io.IOException;
import org.apache.parquet.bytes.BytesInput;
import org.apache.parquet.column.ColumnDescriptor;
import org.apache.parquet.column.page.DataPage;
import org.apache.parquet.column.page.DataPageV1;
import org.apache.parquet.column.page.PageReader;
import org.apache.parquet.column.page.PageWriter;
import org.apache.parquet.column.page.mem.MemPageStore;
Expand All @@ -46,19 +50,42 @@ public void test() throws IOException {
ColumnDescriptor col = new ColumnDescriptor(path, PrimitiveTypeName.INT64, 2, 2);
LongStatistics stats = new LongStatistics();
PageWriter pageWriter = memPageStore.getPageWriter(col);

pageWriter.writePage(BytesInput.from(new byte[735]), 209, stats, BIT_PACKED, BIT_PACKED, PLAIN);
pageWriter.writePage(BytesInput.from(new byte[743]), 209, stats, BIT_PACKED, BIT_PACKED, PLAIN);
pageWriter.writePage(BytesInput.from(new byte[743]), 209, stats, BIT_PACKED, BIT_PACKED, PLAIN);
pageWriter.writePage(BytesInput.from(new byte[735]), 209, stats, BIT_PACKED, BIT_PACKED, PLAIN);

PageReader pageReader = memPageStore.getPageReader(col);
long totalValueCount = pageReader.getTotalValueCount();
LOG.info(String.valueOf(totalValueCount));
LOG.info("Total value count: " + totalValueCount);

assertEquals("Expected total value count to be 836 (4 pages * 209 values)", 836, totalValueCount);

int total = 0;
int pageCount = 0;
do {
DataPage readPage = pageReader.readPage();

// Assert page was successfully read
Copy link
Member

@wgtmac wgtmac Aug 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that TestMemPageStore previously tests nothing but merely to prove that methods of MemPageStore are working.

Copy link
Member Author

@ArnavBalyan ArnavBalyan Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes seems like previouslyMemPageStore was just supposed to test the MemPageStore upper level functionality. Seems that TestMemColumn does the actual data and lower level checks

assertNotNull("Page should not be null", readPage);
// Assert page has expected value count
assertEquals("Each page should have 209 values", 209, readPage.getValueCount());
// Assert encodings when the implementation is DataPageV1
assertTrue("Page should be an instance of DataPageV1", readPage instanceof DataPageV1);
if (readPage instanceof DataPageV1) {
Copy link
Member

@wgtmac wgtmac Aug 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer asserting that this is a DataPageV1

Copy link
Member Author

@ArnavBalyan ArnavBalyan Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the assertion thanks

DataPageV1 v1 = (DataPageV1) readPage;
assertEquals("Page repetition level encoding should be BIT_PACKED", BIT_PACKED, v1.getRlEncoding());
assertEquals("Page definition level encoding should be BIT_PACKED", BIT_PACKED, v1.getDlEncoding());
assertEquals("Page value encoding should be PLAIN", PLAIN, v1.getValueEncoding());
}

total += readPage.getValueCount();
LOG.info(readPage.toString());
// TODO: assert
pageCount++;
} while (total < totalValueCount);

// Assert we read exactly the expected number of pages and values
assertEquals("Should have read 4 pages", 4, pageCount);
assertEquals("Total values read should match totalValueCount", totalValueCount, total);
}
}