Added unit tests for parquet-dataformat plugin #20180

cocosz · 2025-12-07T22:09:14Z

Description

We are adding unit tests for parquet-dataformat plugin classes. In this PR, we are covering :

ParquetWriter
ParquetExecutionEngine
ParquetDocumentInput
ArrowBufferPool

coderabbitai · 2025-12-07T22:09:20Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

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.

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

github-actions · 2025-12-07T22:14:41Z

❌ Gradle check result for 04b25fa: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2025-12-08T10:07:54Z

❌ Gradle check result for d5718c8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

raghuvanshraj · 2025-12-08T14:10:10Z

...ta-format/src/test/java/com/parquet/parquetdataformat/engine/ParquetExecutionEngineTest.java

+ * Unit tests for ParquetExecutionEngine covering engine lifecycle, writer creation,
+ * file loading and tracking, refresh operations, and resource management.
+ */
+public class ParquetExecutionEngineTest extends OpenSearchTestCase {


As discussed, let's rename these classes to end with Tests.java

raghuvanshraj · 2025-12-08T14:17:52Z

...ta-format/src/test/java/com/parquet/parquetdataformat/engine/ParquetExecutionEngineTest.java

+
+    // ===== Basic Engine Functionality Tests =====
+
+    public void testEngineCreationAndInitialization() throws IOException {


I don't see any complex tests with interleaved flows involving document indexing, refreshes, flushes. There should be validations on WriteResult, RefreshResult, WriterFileSet, CatalogSnapshot.

I will cover the scenarios in the next commit

github-actions · 2025-12-11T01:10:55Z

❌ Gradle check result for 72f8c7b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2025-12-12T13:11:03Z

❌ Gradle check result for eaa92a7: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2025-12-16T02:34:58Z

❌ Gradle check result for c461315: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

rayshrey

Can we add all new ParquetDataFormat UTs in the E2E github action ?

rayshrey · 2025-12-16T09:42:16Z

...a-format/src/test/java/com/parquet/parquetdataformat/engine/ParquetExecutionEngineTests.java

+import java.nio.file.*;
+import java.util.*;


Let's avoid * imports

rayshrey · 2025-12-16T09:43:41Z

...a-format/src/test/java/com/parquet/parquetdataformat/engine/ParquetExecutionEngineTests.java

+        super.tearDown();
+    }
+
+    private Schema createRealSchema() {


Nit - DummySchema instead of RealSchema wherever used

rayshrey · 2025-12-17T07:21:10Z

...a-format/src/test/java/com/parquet/parquetdataformat/engine/ParquetExecutionEngineTests.java

+        engine.close();
+    }
+
+    public void testWriterLifecycleAndBasicOperations() throws IOException {


Can we verify end to end parquet file creation as well ?

rayshrey · 2025-12-17T07:21:46Z

...a-format/src/test/java/com/parquet/parquetdataformat/engine/ParquetExecutionEngineTests.java

+        assertEquals(PARQUET_DATA_FORMAT.name(), dataFormat.name());
+    }
+
+    public void testRefreshReturnsEmptyResult() {


Can be skipped for now - serves no purpose.

rayshrey · 2025-12-17T07:26:15Z

...a-format/src/test/java/com/parquet/parquetdataformat/engine/ParquetExecutionEngineTests.java

+        Writer<ParquetDocumentInput> writer = engine.createWriter(generation);
+        assertNotNull(writer);
+
+        ParquetDocumentInput input = writer.newDocumentInput();


Let's create multiple DocumentInputs, add some fields to those document input and then do documentInput.addToWriter for all. Then flush the writer and verify FileInfos returned from flush and also whether the parquet file got created and has all the necessary data that we added - this would really test the entire end to end flow.

rayshrey · 2025-12-17T07:28:38Z

...ata-format/src/test/java/com/parquet/parquetdataformat/writer/ParquetDocumentInputTests.java

+        mockManagedVSR = mock(ManagedVSR.class);
+        mockMappedFieldType = mock(MappedFieldType.class);
+        mockParquetField = mock(ParquetField.class);


Let's create concrete objects instead of mocks here as well.

rayshrey · 2025-12-17T07:47:24Z

...rquet-data-format/src/test/java/com/parquet/parquetdataformat/writer/ParquetWriterTests.java

+        assertSame("AddDoc should return WriteResult from VSRManager", expectedResult, actualResult);
+        verify(vsrManager).addToManagedVSR(input);
+        verifyNoMoreInteractions(vsrManager);


When we are creating a new writer object it will instantiate it's own VSRManager, why are we asserting something on a mock VSRManager ?

Same comment applies to all other places where we are asserting on the mock VSRManager

github-actions · 2025-12-17T09:31:22Z

❌ Gradle check result for 77be482: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

rayshrey · 2025-12-17T09:37:49Z

...a-format/src/test/java/com/parquet/parquetdataformat/engine/ParquetExecutionEngineTests.java

+        assertNotNull(ex.getCause());
+    }
+
+    public void testSupportedFieldTypesReturnsEmptyList() {


Do we aim to remove to utilize this method in future (validation of index creation ?)
If yes, we should skip this test and add a corresponding test later when concrete implementation of supportedFieldTypes is done.

We can remove it for now.

rayshrey · 2025-12-17T09:38:35Z

...a-format/src/test/java/com/parquet/parquetdataformat/engine/ParquetExecutionEngineTests.java

+        assertTrue(supportedTypes.isEmpty());
+    }
+
+    public void testGetMergerReturnsParquetMergeExecutorInstance() {


Let's skip this test as well and take it up in a later PR where we will be adding UTs related to Merges.

Sure we can remove it for now

rayshrey · 2025-12-17T10:26:22Z

...a-format/src/test/java/com/parquet/parquetdataformat/engine/ParquetExecutionEngineTests.java

+    }
+
+    public void testCompleteWriterWorkflowWithMultipleDocuments() throws Exception {
+        long generation = 42L;


Let's move it to a constant and use it

rayshrey · 2025-12-17T10:35:24Z

...a-format/src/test/java/com/parquet/parquetdataformat/engine/ParquetExecutionEngineTests.java

+
+        boolean hasParquetFile = parquetFileSet.getFiles().stream()
+            .anyMatch(f -> f.endsWith(".parquet") && f.contains("_parquet_file_generation_" + generation));
+        assertTrue("Should create Parquet file with correct naming pattern", hasParquetFile);


Let's not just rely on FileInfos but also add a check to ensure physical file exists.

opensearch-trigger-bot · 2026-01-18T15:22:41Z

This PR is stalled because it has been open for 30 days with no activity.

Signed-off-by: Tanvir Alam <[email protected]>

github-actions · 2026-01-20T12:19:47Z

❌ Gradle check result for 3b9c61e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-01-20T13:44:00Z

❌ Gradle check result for 23df254: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-01-20T13:54:38Z

❌ Gradle check result for d3bd66b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-01-20T14:43:59Z

❌ Gradle check result for 0f201aa: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Tanvir Alam <[email protected]>

github-actions · 2026-01-21T06:05:06Z

❌ Gradle check result for 998010c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

cocosz requested a review from a team as a code owner December 7, 2025 22:09

cocosz force-pushed the parquet-test branch from 04b25fa to d5718c8 Compare December 8, 2025 10:02

raghuvanshraj reviewed Dec 9, 2025

View reviewed changes

cocosz force-pushed the parquet-test branch from d5718c8 to 4d90ce1 Compare December 11, 2025 00:54

cocosz changed the title ~~Added unit tests for parquetwriter and parquetexecutionengine classes~~ Added unit tests for parquet-dataformat plugin Dec 11, 2025

cocosz force-pushed the parquet-test branch from 4d90ce1 to 72f8c7b Compare December 11, 2025 00:58

cocosz force-pushed the parquet-test branch from 72f8c7b to eaa92a7 Compare December 12, 2025 13:00

cocosz force-pushed the parquet-test branch from eaa92a7 to c461315 Compare December 16, 2025 02:28

rayshrey reviewed Dec 17, 2025

View reviewed changes

cocosz force-pushed the parquet-test branch from c461315 to 77be482 Compare December 17, 2025 09:24

rayshrey reviewed Dec 17, 2025

View reviewed changes

opensearch-trigger-bot bot added the stalled Issues that have stalled label Jan 18, 2026

Tanvir Alam added 4 commits January 20, 2026 16:28

Added unit tests for parquetwriter and parquetexecutionengine classes

53bed90

Signed-off-by: Tanvir Alam <[email protected]>

Added unit tests to cover e2e flow

4184b46

Signed-off-by: Tanvir Alam <[email protected]>

removed excessive mocking from unit tests

de726cd

Signed-off-by: Tanvir Alam <[email protected]>

Added changes to test using real objects

6e29e22

Signed-off-by: Tanvir Alam <[email protected]>

cocosz force-pushed the parquet-test branch from 77be482 to 3b9c61e Compare January 20, 2026 12:10

cocosz force-pushed the parquet-test branch from 3b9c61e to 23df254 Compare January 20, 2026 13:38

cocosz force-pushed the parquet-test branch from 23df254 to d3bd66b Compare January 20, 2026 13:44

cocosz force-pushed the parquet-test branch from d3bd66b to 0f201aa Compare January 20, 2026 14:38

cocosz force-pushed the parquet-test branch from 0f201aa to 9c4c16b Compare January 21, 2026 05:48

added changes to formalize tests

998010c

Signed-off-by: Tanvir Alam <[email protected]>

cocosz force-pushed the parquet-test branch from 9c4c16b to 998010c Compare January 21, 2026 05:52


		// ===== Basic Engine Functionality Tests =====

		public void testEngineCreationAndInitialization() throws IOException {

		import java.nio.file.*;
		import java.util.*;

Added unit tests for parquet-dataformat plugin #20180

Are you sure you want to change the base?

Added unit tests for parquet-dataformat plugin #20180

Uh oh!

Conversation

cocosz commented Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

coderabbitai bot commented Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions bot commented Dec 7, 2025

Uh oh!

github-actions bot commented Dec 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 11, 2025

Uh oh!

github-actions bot commented Dec 12, 2025

Uh oh!

github-actions bot commented Dec 16, 2025

Uh oh!

rayshrey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-trigger-bot bot commented Jan 18, 2026

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

github-actions bot commented Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

cocosz commented Dec 7, 2025 •

edited

Loading

coderabbitai bot commented Dec 7, 2025 •

edited

Loading