Skip to content

[#8921] improvement(catalogs): Add ITs for lance table operations.#8923

Merged
yuqi1129 merged 5 commits intoapache:branch-lance-namepspace-devfrom
yuqi1129:issue_8921
Oct 28, 2025
Merged

[#8921] improvement(catalogs): Add ITs for lance table operations.#8923
yuqi1129 merged 5 commits intoapache:branch-lance-namepspace-devfrom
yuqi1129:issue_8921

Conversation

@yuqi1129
Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Add some ITs to covert lance table operations and fix a bug by the way.

Why are the changes needed?

It's a improvement.

Fix: #8921

Does this PR introduce any user-facing change?

N/A.

How was this patch tested?

It's self just the test.

@yuqi1129 yuqi1129 requested review from jerryshao and mchades October 27, 2025 15:47
mchades
mchades previously approved these changes Oct 28, 2025
@mchades mchades dismissed their stale review October 28, 2025 06:12

one comment

implementation(libs.commons.lang3)
implementation(libs.guava)
implementation(libs.hadoop3.client.api)
implementation(libs.hadoop3.client.runtime)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why need to add this?

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.

In the method dropTable, the following code will lack necessary dependencies if there exists only hadoop-client-api

      FileSystem fs = FileSystem.get(new Configuration());
      return fs.delete(new Path(location), true);

They should show up together.

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.

I will optimize this part in another PR and remove the use of FileSystem here.

public class TableVersionPostgreSQLProvider extends TableVersionBaseSQLProvider {

public class TableVersionPostgreSQLProvider extends TableVersionBaseSQLProvider {}
public String insertTableVersionOnDuplicateKeyUpdate(@Param("tablePO") TablePO tablePO) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add UTs for this part?

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.

Added

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class CatalogGenericLakeLanceIT extends BaseIT {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lake or Lakehouse?

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.

Fixed


// Note: GenericTableEntity will be removed in the refactor PR, so here just keep the old
// logic to make the UT pass.
if (newTable instanceof GenericTableEntity genericTable) {
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 a temporary change to make the CI pass, and #8922 will remove the class GenericTableEntity and use TableEntity.

@yuqi1129
Copy link
Copy Markdown
Contributor Author

@jerryshao @mchades
All comments have been resolved, please look again, thanks.

Copy link
Copy Markdown
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

lgtm.

@yuqi1129 yuqi1129 merged commit d0c58f9 into apache:branch-lance-namepspace-dev Oct 28, 2025
26 checks passed
jerryshao pushed a commit to jerryshao/gravitino that referenced this pull request Nov 11, 2025
…ns. (apache#8923)

### What changes were proposed in this pull request?

Add some ITs to covert lance table operations and fix a bug by the way. 

### Why are the changes needed?

It's a improvement.

Fix: apache#8921 

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

It's self just the test.
youngyjd pushed a commit to youngyjd/gravitino that referenced this pull request Nov 20, 2025
…ns. (apache#8923)

### What changes were proposed in this pull request?

Add some ITs to covert lance table operations and fix a bug by the way. 

### Why are the changes needed?

It's a improvement.

Fix: apache#8921 

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

It's self just the test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants