Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zzzk1
Copy link
Contributor

@zzzk1 zzzk1 commented Feb 26, 2025

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

+ " AND mm.deleted_at = 0 AND cm.deleted_at = 0";
}

public String listCatalogPOsByCatalogName(@Param("catalogName") String catalogName) {
Copy link
Contributor

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?

Copy link
Contributor Author

@zzzk1 zzzk1 left a 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.

Comment on lines 130 to 142
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)));
}
Copy link
Contributor Author

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.

@zzzk1
Copy link
Contributor Author

zzzk1 commented Feb 27, 2025

Maybe I have found the reason: currently, the IT test only contains level-0 conditions. That is why the incorrect refactor still passes CI.

CatalogEntity catalog =
createCatalog(
RandomIdGenerator.INSTANCE.nextId(),
NamespaceUtil.ofCatalog("metalake"),

backend.list(catalog.namespace(), Entity.EntityType.CATALOG, true);
assertTrue(catalogs.contains(catalog));

@zzzk1 zzzk1 changed the title [#4024]refactor: Catalog storage queries using JOIN for JDBC [#4024]Refactor: Reduce unnecessary queries in catalog JDBC implementation Feb 27, 2025
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.

2 participants