Skip to content

test(java): re-construct tests for merge operation#4483

Merged
jackye1995 merged 14 commits intolance-format:mainfrom
majin1102:java-test-refactor
Aug 25, 2025
Merged

test(java): re-construct tests for merge operation#4483
jackye1995 merged 14 commits intolance-format:mainfrom
majin1102:java-test-refactor

Conversation

@majin1102
Copy link
Copy Markdown
Contributor

@majin1102 majin1102 commented Aug 15, 2025

I found merge operation test is not right. It didn't construct by merging a new column, but instead merged some new rows. Although it works, it's not what we wanted.

In the meanwhile the TransactionTest has grown a little tedious and not that friendly to construct multiple testcases on a single operation. So I did a little refactoring.

The work includes:

  1. Move all operation testcases into package com/lancedb/lance/operation.
  2. Extend some new testcase for Append and Merge operation. We could do similar work to other operations.
  3. Original TransactionTest is only used to test transaction framework like readTransaction and transaction properties.

@majin1102 majin1102 changed the title test(java): add tests for merge operation test(java): re-construct tests for merge operation Aug 20, 2025
@majin1102
Copy link
Copy Markdown
Contributor Author

@jackye1995 PTAL when you have time

@majin1102
Copy link
Copy Markdown
Contributor Author

majin1102 commented Aug 25, 2025

@yanghua Could you take a look when you have time?
This is only test releated but could block next transaction steps

Copy link
Copy Markdown
Collaborator

@yanghua yanghua left a comment

Choose a reason for hiding this comment

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

Left two comments.


@Test
void testReplaceAsDiffColumns(@TempDir Path tempDir) throws Exception {
String datasetPath = tempDir.resolve("testMergeNewColumn").toString();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wrong dir name?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks for this


public static final int TEST_FILE_FORMAT_MAJOR_VERSION = 2;
public static final int TEST_FILE_FORMAT_MINOR_VERSION = 0;
protected static Dataset dataset;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Must it be static?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is only a movement.
We can make it better with @testinstance(TestInstance.Lifecycle.PER_CLASS)
Please take another look

Copy link
Copy Markdown
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

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

Nice refactoring! looks good to me

@jackye1995 jackye1995 merged commit 709be73 into lance-format:main Aug 25, 2025
7 checks passed
@majin1102 majin1102 deleted the java-test-refactor branch September 10, 2025 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants