-
Notifications
You must be signed in to change notification settings - Fork 406
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.
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
[#4024]Refactor: Reduce unnecessary queries in catalog JDBC implementation #6540
base: main
Are you sure you want to change the base?
Conversation
+ " AND mm.deleted_at = 0 AND cm.deleted_at = 0"; | ||
} | ||
|
||
public String listCatalogPOsByCatalogName(@Param("catalogName") String catalogName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a scenario which we need to get all catalogs with the name catalogName
in all metalakes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuqi1129 The refactor was incorrect, but the CI has passed. It is possible that we overlooked many conditions that were not checked.
if (namespace.levels().length >= 2) { | ||
catalogPOS = | ||
SessionUtils.getWithoutCommit( | ||
CatalogMetaMapper.class, | ||
mapper -> mapper.listCatalogPOsByCatalogName(namespace.level(1))); | ||
} | ||
|
||
Long metalakeId = CommonMetaService.getInstance().getParentEntityIdByNamespace(namespace); | ||
|
||
List<CatalogPO> catalogPOS = | ||
SessionUtils.getWithoutCommit( | ||
CatalogMetaMapper.class, mapper -> mapper.listCatalogPOsByMetalakeId(metalakeId)); | ||
if (namespace.levels().length >= 3) { | ||
catalogPOS = | ||
SessionUtils.getWithoutCommit( | ||
CatalogMetaMapper.class, | ||
mapper -> mapper.listCatalogPOsBySchemaName(namespace.level(2))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mistakes here right?metalake has many catalogs, catalog has many schema. level-1 needs metalake name
catalog name
, level-2 needs metalake name
catalog name
schema name
.
Maybe I have found the reason: currently, the IT test only contains level-0 conditions. That is why the incorrect refactor still passes CI. gravitino/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java Lines 565 to 568 in 8ac10ce
gravitino/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java Lines 759 to 760 in 8ac10ce
|
What changes were proposed in this pull request?
For queries involving more related tables, use a JOIN operation instead of executing separate queries to retrieve the results.
Why are the changes needed?
There are many unnecessary query operations, and the two tables can be directly associated to reduce these operations.
Part of: #4024
Does this PR introduce any user-facing change?
no
How was this patch tested?
unit tests & backend intergation tests