[#6744] improvment(core): Batch get metadata objects for TagMetaService #6752

yuqi1129 · 2025-03-25T13:53:53Z

What changes were proposed in this pull request?

Group by metadata object by metadata type and batch get all metadata with the same type.

Why are the changes needed?

To return the times to access databases and improve performance.

Fix: #6744

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Test locally and existing tests.

yuqi1129 · 2025-03-28T06:28:20Z

@jerqi

Could you please take some time to review this PR?

jerryshao · 2025-03-28T21:45:17Z

core/src/main/java/org/apache/gravitino/storage/relational/mapper/TableColumnMapper.java

@@ -71,4 +71,7 @@ Long selectColumnIdByTableIdAndName(

  @SelectProvider(type = TableColumnSQLProviderFactory.class, method = "selectColumnPOById")
  ColumnPO selectColumnPOById(@Param("columnId") Long columnId);
+
+  @SelectProvider(type = TableColumnSQLProviderFactory.class, method = "listColumnPOsByColumnIds")
+  List<ColumnPO> listColumnPOsByTableIds(@Param("columnIds") List<Long> columnIds);


columnIds or TableIds?

It's columnIds and I will modify it.

jerryshao · 2025-03-28T21:50:20Z

...org/apache/gravitino/storage/relational/mapper/provider/base/TableColumnBaseSQLProvider.java

+        + ") "
+        + " AND c.deleted_at = 0"
+        + "</script>";
+  }


Does this SQL work for H2 and PG?

Can you add a test to cover this?

TagIT has tests tag.associatedObjects() to cover this changes and It should be work for H2 and PG as the IT has been run with backend H2 and PG.

jerryshao · 2025-03-28T21:59:31Z

core/src/main/java/org/apache/gravitino/storage/relational/service/MetadataObjectService.java

+            LOG.warn(
+                "The table '{}' of column '{}' may be deleted",
+                columnPO.getTableId(),
+                columnPO.getColumnId());
+            columnIdAndNameMap.put(columnPO.getColumnId(), null);
+            return;


What will be happened if table name is null? Shall we throw an exception here?

According to the comment and code logic, the table can be deleted when when query the columns, we should ingore it. please see:

gravitino/core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java

Lines 238 to 242 in 2668aaf

// Metadata object may be deleted asynchronously when we query the name, so it will return

// null. We should skip this metadata object.

if (fullName == null) {

continue;

}

jerryshao · 2025-03-28T22:02:39Z

core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java

+                        metadataObjects.add(MetadataObjects.parse(fullName, type));
+                      }
+                    });
+              });


Do we have a test to cover this, if not please add a test.

There exists UT that can covert it and I have checked several problems during developing process through it, please see TestTagMetaService#testListAssociatedMetadataObjectsForTag

I would suggest changing to use for..loop here instead of using stream, it is very hard to read the code.

jerqi · 2025-03-31T02:33:09Z

core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java

@@ -55,6 +58,18 @@ public static TagMetaService getInstance() {
    return INSTANCE;
  }

+  private static final Map<MetadataObject.Type, Function<List<Long>, Map<Long, String>>>


Shall RoleService replace the code with similar logic?

Yes, the code in RoleService uses a temporary value in a method to solve it. They are mostly the same and have minor differences.

Do you mean we may need to try to merge them?

Yes, we should merge them. I guess

jerryshao · 2025-03-31T17:15:48Z

.../main/java/org/apache/gravitino/storage/relational/mapper/TableColumnSQLProviderFactory.java

@@ -90,4 +90,8 @@ public static String selectColumnIdByTableIdAndName(
  public static String selectColumnPOById(@Param("columnId") Long columnId) {
    return getProvider().selectColumnPOById(columnId);
  }
+
+  public static String listColumnPOsByColumnIds(@Param("columnIds") List<Long> columnIds) {
+    return getProvider().listColumnPOsByTableIds(columnIds);


Is it "columnIds" or tableIds?

jerryshao · 2025-03-31T17:20:09Z

core/src/main/java/org/apache/gravitino/storage/relational/service/MetadataObjectService.java

@@ -57,6 +59,18 @@ public class MetadataObjectService {

  private static final Logger LOG = LoggerFactory.getLogger(MetadataObjectService.class);

+  public static final Map<MetadataObject.Type, Function<List<Long>, Map<Long, String>>>


Why do you make it public?

RoleMetaService also uses it. I can lower the access level and use the package level.

jerryshao

LGTM.

…aService (apache#6752) ### What changes were proposed in this pull request? Group by metadata object by metadata type and batch get all metadata with the same type. ### Why are the changes needed? To return the times to access databases and improve performance. Fix: apache#6744 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? Test locally and existing tests.

Batch get metadata objects for TagMetaService

0e6509a

yuqi1129 self-assigned this Mar 25, 2025

yuqi1129 added 2 commits March 25, 2025 22:52

fix

6a567a0

remove some unused code.

bc58c3f

yuqi1129 force-pushed the issue_6744 branch from 063d9d7 to bc58c3f Compare March 26, 2025 06:47

yuqi1129 requested a review from jerqi March 26, 2025 09:07

jerryshao reviewed Mar 28, 2025

View reviewed changes

Fix comments

866fcb8

jerqi reviewed Mar 31, 2025

View reviewed changes

Merge functions in TagMetaService and RoleMetaService

67ae658

jerryshao reviewed Mar 31, 2025

View reviewed changes

Resolve comments

9b54f03

jerryshao approved these changes Apr 4, 2025

View reviewed changes

jerryshao merged commit fbdb14f into apache:main Apr 4, 2025
28 checks passed

	// Metadata object may be deleted asynchronously when we query the name, so it will return
	// null. We should skip this metadata object.
	if (fullName == null) {
	continue;
	}

		@@ -57,6 +59,18 @@ public class MetadataObjectService {

		private static final Logger LOG = LoggerFactory.getLogger(MetadataObjectService.class);

		public static final Map<MetadataObject.Type, Function<List<Long>, Map<Long, String>>>

[#6744] improvment(core): Batch get metadata objects for TagMetaService #6752

[#6744] improvment(core): Batch get metadata objects for TagMetaService #6752

Uh oh!

Conversation

yuqi1129 commented Mar 25, 2025

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

yuqi1129 commented Mar 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryshao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!