Add table metadata service #2434

inv-jishnu · 2024-12-19T10:02:45Z

Description

Added table metadata service and some related DTO classes

Related issues and/or PRs

NA

Changes made

Added table metadata service and some related DTO classes.
The tablemetadata service is also used in data loader API.
The spotbug exlude is the same as the one added in other PR to resolve issue due Lombok.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

Road map to merge remaining data loader core files. Current status

General
- Add ScalarDB Dao and related files - Add ScalarDB Dao and related files #2417
- TableMetadataService(partially replaced by ConsensusUtils): Add table metadata service #2434
Export
- Export options and validations: Add export options validator #2435
- ProducerTasks: 1 PR incoming
Import
- Dto classes and utilities: 1 PR incoming
- Import processor and task code: 2-3 PRs incoming
- Code for Import transaction batch and data chunk import: 1 PR Incoming
- ControlFile related Dtos: 1 PR incoming
- Import logger: 1 PR incoming
- Import Manager: 1 PR incoming

Release notes

NA

komamitsu · 2024-12-20T01:58:33Z

...ore/src/main/java/com/scalar/db/dataloader/core/dataimport/controlfile/ControlFileTable.java

+
+  /** The namespace of the table in ScalarDB. */
+  @JsonProperty("namespace")
+  private String namespace;


[minor] This seems the name of a namespace as well as the name of a table below. So, this should be namespaceName for consistency?

komamitsu · 2024-12-20T02:00:28Z

...der/core/src/main/java/com/scalar/db/dataloader/core/tablemetadata/TableMetadataRequest.java

+@Getter
+public class TableMetadataRequest {
+
+  private final String namespace;


[minor] Same as above

komamitsu · 2024-12-20T02:11:21Z

data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/TableMetadataUtil.java

+   *
+   * @return A set of predefined metadata column names.
+   */
+  public static Set<String> getMetadataColumns() {


How about using com.scalar.db.transaction.consensuscommit.ConsensusCommitUtils#getTransactionMetaColumns to avoid duplicated logic?

Some other methods might be replaced with ConsensusCommitUtils methods.

brfrn169

Left a couple comments. Please take a look when you have time!

brfrn169 · 2024-12-20T04:35:22Z

data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/TableMetadataUtil.java

+   * @param columnNames A set of all column names in the table.
+   * @return {@code true} if the column is a metadata column; {@code false} otherwise.
+   */
+  public static boolean isMetadataColumn(


You can use ConsensusCommitUtils.isTransactionMetaColumn():

scalardb/core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitUtils.java

Line 201 in 98b5e1b

public static boolean isTransactionMetaColumn(String columnName, TableMetadata tableMetadata) {

How about this? Have you made a change for this?

brfrn169 · 2024-12-20T04:35:46Z

data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/TableMetadataUtil.java

+   * @param tableMetadata The metadata of the table.
+   * @return {@code true} if the column is a metadata column; {@code false} otherwise.
+   */
+  public static boolean isMetadataColumn(String columnName, TableMetadata tableMetadata) {


inv-jishnu · 2025-01-16T12:22:11Z

@brfrn169 san, @brfrn169 san,

I have made changes based on your feedback.
Please take a look at this again when you get a chance.
Thank you

komamitsu

LGTM! 👍

brfrn169 · 2025-01-17T02:35:43Z

...der/core/src/main/java/com/scalar/db/dataloader/core/tablemetadata/TableMetadataService.java

+  private static final String ERROR_MISSING_NAMESPACE_OR_TABLE =
+      "Missing namespace or table: %s, %s";


Please move this to CoreError. Or we might already have the same message for it in CoreError. If so, pease use it.

brfrn169 · 2025-01-17T02:38:20Z

data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/TableMetadataUtil.java

+   * @param columnNames A set of all column names in the table.
+   * @return {@code true} if the column is a metadata column; {@code false} otherwise.
+   */
+  public static boolean isMetadataColumn(


How about this? Have you made a change for this?

inv-jishnu · 2025-01-20T06:05:00Z

@brfrn169 san,

I have removed the code for isMetadataColumn and moved the error message to CoreError class.
Please take a look at this again when you get a chance.
Thank you.

brfrn169

Left a couple of comments. Please take a look!

brfrn169 · 2025-01-24T04:49:37Z

...der/core/src/main/java/com/scalar/db/dataloader/core/tablemetadata/TableMetadataService.java

+      throw new TableMetadataException(
+          CoreError.DATA_LOADER_MISSING_NAMESPACE_OR_TABLE.buildMessage(namespace, tableName),
+          e.getCause());


It looks like this exception is thrown when retrieving table metadata fails. In that case, is this error message appropriate?

Thank you.
I have added another message.

brfrn169 · 2025-01-24T04:52:55Z

data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/TableMetadataUtil.java

+  public static Map<String, DataType> extractColumnDataTypes(TableMetadata tableMetadata) {
+    Map<String, DataType> definitions = new HashMap<>();
+    for (String columnName : tableMetadata.getColumnNames()) {
+      definitions.put(columnName, tableMetadata.getColumnDataType(columnName));
+    }
+    return definitions;
+  }


I just realized it but we can use TableMetadata.getColumnDataTypes() for this.

I have removed this code and will use TableMetadata.getColumnDataTypes() instead.
Thank you.

brfrn169 · 2025-01-24T12:32:51Z

core/src/main/java/com/scalar/db/common/error/CoreError.java

+  DATA_LOADER_MISSING_NAMESPACE_OR_TABLE(
+      Category.USER_ERROR, "0165", "Missing namespace or table: %s, %s", "", ""),
+  DATA_LOADER_TABLE_METADATA_RETRIEVAL_FAILED(
+      Category.USER_ERROR, "0166", "Failed to retrieve table metadata. Details:  %s", "", ""),


Nit:

Suggested change

Category.USER_ERROR, "0166", "Failed to retrieve table metadata. Details: %s", "", ""),

Category.USER_ERROR, "0166", "Failed to retrieve table metadata. Details: %s", "", ""),

brfrn169 · 2025-01-24T12:36:12Z

...der/core/src/main/java/com/scalar/db/dataloader/core/tablemetadata/TableMetadataService.java

+    } catch (ExecutionException e) {
+      throw new TableMetadataException(
+          CoreError.DATA_LOADER_TABLE_METADATA_RETRIEVAL_FAILED.buildMessage(e.getMessage()),
+          e.getCause());


Why e.getCause() instead of e?

Suggested change

e.getCause());

e);

inv-jishnu · 2025-01-27T11:46:57Z

@brfrn169 san,
I have made changes as suggested.
Please take a look at this again when you get a chance.
Thank you.

Torch3333

LGTM, thank you.

Torch3333 · 2025-01-28T08:49:50Z

core/src/main/java/com/scalar/db/common/error/CoreError.java

+  DATA_LOADER_MISSING_NAMESPACE_OR_TABLE(
+      Category.USER_ERROR, "0165", "Missing namespace or table: %s, %s", "", ""),
+  DATA_LOADER_TABLE_METADATA_RETRIEVAL_FAILED(
+      Category.USER_ERROR, "0166", "Failed to retrieve table metadata. Details: %s", "", ""),


When updating or creating error messages. Please explicitly ask @josh-wong to review them to check the English phrasing.
@josh-wong Please check the error message when you get the chance.

josh-wong

LGTM! Thank you!🙇🏻‍♂️

brfrn169

LGTM! Thank you!

inv-jishnu added 5 commits December 17, 2024 18:05

Initial commit [skip ci]

2b52eeb

Added a file [skip ci]

90c4105

Added unit test files [skip ci]

3d5d3e0

Spotbug fixes

6495202

Merge branch 'master' into feat/data-loader/metadata-service

b1b811b

inv-jishnu added the enhancement New feature or request label Dec 19, 2024

inv-jishnu marked this pull request as draft December 19, 2024 10:02

Applied spotless

30db988

inv-jishnu self-assigned this Dec 19, 2024

ypeckstadt mentioned this pull request Dec 19, 2024

Add ScalarDB Dao and related files #2417

Open

6 tasks

ypeckstadt requested review from komamitsu, brfrn169, feeblefakie and Torch3333 December 19, 2024 23:47

ypeckstadt marked this pull request as ready for review December 19, 2024 23:48

ypeckstadt mentioned this pull request Dec 19, 2024

Add export options validator #2435

Merged

6 tasks

ypeckstadt added 3 commits December 20, 2024 09:02

Improve javadocs

ccb1ace

Add private constructor to TableMetadataUtil

a374f1a

Apply spotless fix

a65c9b5

komamitsu reviewed Dec 20, 2024

View reviewed changes

brfrn169 reviewed Dec 20, 2024

View reviewed changes

inv-jishnu mentioned this pull request Dec 24, 2024

Add data chunk and task result enums and dtos #2442

Open

6 tasks

This was referenced Jan 2, 2025

Add Control file module files and validation #2445

Draft

Add dtos and other classes for task #2446

Draft

Add export tasks #2450

Draft

Merge branch 'master' into feat/data-loader/metadata-service

a9f767d

This was referenced Jan 10, 2025

Add import processes #2462

Draft

Add import log classes and utils #2463

Draft

Changes

3b76653

inv-jishnu requested a review from brfrn169 January 16, 2025 12:20

inv-jishnu requested a review from komamitsu January 16, 2025 12:20

komamitsu approved these changes Jan 17, 2025

View reviewed changes

brfrn169 reviewed Jan 17, 2025

View reviewed changes

inv-jishnu and others added 2 commits January 20, 2025 10:14

Changes update

d984eea

Merge branch 'master' into feat/data-loader/metadata-service

153c2d1

inv-jishnu requested a review from brfrn169 January 20, 2025 06:05

Merged master and resolved conflicts

9ad8d4e

brfrn169 reviewed Jan 24, 2025

View reviewed changes

inv-jishnu and others added 2 commits January 24, 2025 15:42

Changes

4f1aff8

Merge branch 'master' into feat/data-loader/metadata-service

9a4a45a

inv-jishnu requested a review from brfrn169 January 24, 2025 11:01

brfrn169 reviewed Jan 24, 2025

View reviewed changes

Changes

cb6e63a

inv-jishnu requested a review from brfrn169 January 27, 2025 11:42

Torch3333 approved these changes Jan 28, 2025

View reviewed changes

Torch3333 requested a review from josh-wong January 28, 2025 08:55

josh-wong approved these changes Jan 28, 2025

View reviewed changes

brfrn169 approved these changes Jan 29, 2025

View reviewed changes

This was referenced Jan 29, 2025

Add import command for data loader CLI #2489

Draft

Add export command for data loader CLI #2490

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add table metadata service #2434

Add table metadata service #2434

inv-jishnu commented Dec 19, 2024 •

edited by ypeckstadt

Loading

komamitsu Dec 20, 2024

komamitsu Dec 20, 2024

komamitsu Dec 20, 2024

brfrn169 left a comment

brfrn169 Dec 20, 2024

brfrn169 Jan 17, 2025

brfrn169 Dec 20, 2024

inv-jishnu commented Jan 16, 2025

komamitsu left a comment

brfrn169 Jan 17, 2025

brfrn169 Jan 17, 2025

inv-jishnu commented Jan 20, 2025

brfrn169 left a comment

brfrn169 Jan 24, 2025

inv-jishnu Jan 24, 2025

brfrn169 Jan 24, 2025

inv-jishnu Jan 24, 2025

brfrn169 Jan 24, 2025

brfrn169 Jan 24, 2025

inv-jishnu commented Jan 27, 2025

Torch3333 left a comment

Torch3333 Jan 28, 2025

josh-wong left a comment

brfrn169 left a comment

		private static final String ERROR_MISSING_NAMESPACE_OR_TABLE =
		"Missing namespace or table: %s, %s";

	Category.USER_ERROR, "0166", "Failed to retrieve table metadata. Details: %s", "", ""),
	Category.USER_ERROR, "0166", "Failed to retrieve table metadata. Details: %s", "", ""),

Add table metadata service #2434

Are you sure you want to change the base?

Add table metadata service #2434

Conversation

inv-jishnu commented Dec 19, 2024 • edited by ypeckstadt Loading

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inv-jishnu commented Jan 16, 2025

komamitsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inv-jishnu commented Jan 20, 2025

brfrn169 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inv-jishnu commented Jan 27, 2025

Torch3333 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh-wong left a comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

inv-jishnu commented Dec 19, 2024 •

edited by ypeckstadt

Loading