[#9298] improvement(lance-rest): Add more tests in Lance REST and lakehouse generic catalog.#9299
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes bugs in the Lance REST server and lakehouse generic catalog related to the table registration configuration. The changes standardize the configuration key from the non-namespaced register to the properly namespaced lance.register, and ensure that registered tables are marked as external tables by setting the external property to "true".
Key Changes
- Introduced centralized constants
LANCE_TABLE_FORMATandLANCE_TABLE_REGISTERinLanceConstantsclass - Updated
registerTableoperations to use the namespacedlance.registerproperty key instead of plainregister - Modified both REST and catalog implementations to set the
externalproperty to "true" for registered tables
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/utils/LanceConstants.java |
Added LANCE_TABLE_FORMAT and LANCE_TABLE_REGISTER constants for centralized configuration key management |
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java |
Updated to use LANCE_TABLE_REGISTER constant instead of hardcoded "register" string when setting table registration property |
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java |
Modified registerTable to set both PROPERTY_TABLE_FORMAT and PROPERTY_EXTERNAL for registered tables using constants |
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java |
Updated to reference LanceConstants.LANCE_TABLE_REGISTER instead of the removed local constant |
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableDelegator.java |
Refactored to remove duplicate constant definitions and use centralized constants from LanceConstants |
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java |
Added assertions to verify registered tables don't create physical directories initially |
Comments suppressed due to low confidence (1)
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:621
- The test verifies the location and other properties but doesn't validate that the
externalproperty is set to "true" for registered tables, which is a key part of this bug fix (see PR description). Consider adding an assertion to verify this:
Assertions.assertEquals("true", loadTable.getProperties().get("external")); DescribeTableRequest describeTableRequest = new DescribeTableRequest();
describeTableRequest.setId(ids);
DescribeTableResponse loadTable = ns.describeTable(describeTableRequest);
Assertions.assertNotNull(loadTable);
Assertions.assertEquals(location, loadTable.getLocation());
Assertions.assertTrue(loadTable.getProperties().containsKey("key1"));
...main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
Outdated
Show resolved
Hide resolved
|
I've fixed some of the issues in another PR. The thing is that we don't have UTs/ITs to reflect the issue, or the issue is silent when we do the changes. So the key thing is that we need UTs to cover the code. |
Okay, I will hold this PR and activate it based on later fixes. |
|
No, I mean I've fixed several bugs in this #9279 , so you don't need so much changes here. Just adding more tests to cover the code. |
register and external in Lance REST and lakehouse generic catalog.
What changes were proposed in this pull request?
registertolance.registerin register a lance table.externalto true for a registered table.Why are the changes needed?
It's a bug.
Fix: #9298
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
IT and existing tests.