Add data loader core key and column utils #1771

ypeckstadt · 2024-05-26T01:31:47Z

Description

Later in the code for the export and import managers, we need a lot of utils that help us deal with working and converting ScalarDB keys and columns. This PR adds the basic version of the utils class, allowing us to convert keys based on CLI input in the following PR. More methods will be added to these utils later.

Related issues and/or PRs

This PR depends on the following PRs and should be reviewed once merged.

Changes made

Added KeyUtils class
Added ColumnUtils class
Added exceptions thrown by the utils
Added unit test for both utils
Added error messages to the ScalarDB CoreError

Checklist

The following is a best-effort checklist. If any items in this checklist do not apply 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)

NA

Release notes

NA

…utils

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

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

komamitsu · 2024-05-28T10:32:31Z

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

+ DataType columnDataType = tableMetadata.getColumnDataType(columnName);
+ if (columnDataType == null) {
+ throw new KeyParsingException(
+ CoreError.DATA_LOADER_INVALID_COLUMN_KEY_PARSING_FAILED.buildMessage(columnName));


I'm wondering what if there are 3 tables that have name column and this exception is thrown for the second table. Users may want to know which table...? What do you think?

Updated the method and now passing in the table name as well. I had to add it because the table name is not part of the TableMetadata.

@ypeckstadt Thanks for improving the error message!

But, sorry, I should've noticed this when posting the previous comment, other error messages (e.g., DATA_LOADER_INVALID_COLUMN_NON_EXISTENT) have the same issue. Also, the namespace name should be added as well. I think we have 2 options as follows:

Add namespace and table names to all the error messages if needed

The class is a util class for keys, so it may be okay to include only key name in the error messages from KeyUtils. Instead, we'll catch exceptions from KeyUtils and wrap it with the namespace and table names like this:

try { : // This exception doesn't contain the namespace or table name. Key key = KeyUtils.parseKeyValue(columnKeyValue, tableMetadata); } catch (KeyParsingException e) { // Add the namespace and table names information. throw new KeyParsingException( CoreError.DATA_LOADER_XXXX_FAILED.buildMessage(namespaceName, tableName), e); }

I think either is fine. What do you think?

I have made changes based on the above feedback. I went for option 1 as it seems like the safest solution. Option 2 is good as well, but then it shifts the responsibility for proper exception handling and logging to all classes that use the Key Utils. So it creates the risk that some classes do it and some don't.

Thanks for the improvement!

It looks like there is room to improve the other error messages from the same perspective (i.e., missing information about namespace and table). What do you think?

@komamitsu My apologies for the late reply. (had to focus on ScalarFlow more). I have updated the PR and ensured each exception message includes the namespace and table name.

To avoid having to pass in all 3 with all methods, I have added a wrapper class called ColumnInfo that includes all 3 fields.

brfrn169

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

brfrn169 · 2024-05-29T05:21:35Z

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

+ * @return ScalarDB column
+ * @throws Base64Exception if an error occurs while base64 decoding
+ */
+ public static Column<?> createColumnFromValue(DataType dataType, String columnName, String value)


Adding ＠Nullable for nullable parameters would be helpful:

Suggested change

public static Column<?> createColumnFromValue(DataType dataType, String columnName, String value)

public static Column<?> createColumnFromValue(DataType dataType, String columnName, ＠Nullable String value)

brfrn169 · 2024-05-29T05:23:52Z

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

+ } catch (NumberFormatException e) {
+ throw new NumberFormatException(
+ CoreError.DATA_LOADER_INVALID_NUMBER_FORMAT_FOR_COLUMN_VALUE.buildMessage(columnName));
+ } catch (IllegalArgumentException e) {
+ throw new Base64Exception(
+ CoreError.DATA_LOADER_INVALID_BASE64_ENCODING_FOR_COLUMN_VALUE.buildMessage(columnName));
+ }


I think we should set the cause of the exceptions:

Suggested change

} catch (NumberFormatException e) {

throw new NumberFormatException(

CoreError.DATA_LOADER_INVALID_NUMBER_FORMAT_FOR_COLUMN_VALUE.buildMessage(columnName));

} catch (IllegalArgumentException e) {

throw new Base64Exception(

CoreError.DATA_LOADER_INVALID_BASE64_ENCODING_FOR_COLUMN_VALUE.buildMessage(columnName));

}

} catch (NumberFormatException e) {

throw new NumberFormatException(

CoreError.DATA_LOADER_INVALID_NUMBER_FORMAT_FOR_COLUMN_VALUE.buildMessage(columnName), e);

} catch (IllegalArgumentException e) {

throw new Base64Exception(

CoreError.DATA_LOADER_INVALID_BASE64_ENCODING_FOR_COLUMN_VALUE.buildMessage(columnName), e);

}

Added. Although to fix this I had to revise the used exceptions a bit.

brfrn169 · 2024-05-29T05:24:44Z

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

+ throw new KeyParsingException(
+ CoreError.DATA_LOADER_INVALID_VALUE_KEY_PARSING_FAILED.buildMessage(
+ columnKeyValue.getColumnValue(), tableName, e.getMessage()));


Ditto.

Suggested change

throw new KeyParsingException(

CoreError.DATA_LOADER_INVALID_VALUE_KEY_PARSING_FAILED.buildMessage(

columnKeyValue.getColumnValue(), tableName, e.getMessage()));

throw new KeyParsingException(

CoreError.DATA_LOADER_INVALID_VALUE_KEY_PARSING_FAILED.buildMessage(

columnKeyValue.getColumnValue(), tableName, e.getMessage()), e);

Torch3333

LGTM, thank you!

komamitsu · 2024-06-10T06:46:20Z

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

+ "Invalid key: Column %s does not exist in the table %s in namespace %s.",
+ "",
+ ""),
+ DATA_LOADER_INVALID_VALUE_KEY_PARSING_FAILED(


This doesn't seem to be used?

My bad. It has become unused due to refactoring the error handling based on Toshi's feedback. I have removed it now.

brfrn169

Left several very minor comments. Other than that, LGTM! Thank you!

brfrn169 · 2024-07-09T01:29:55Z

data-loader/build.gradle

@@ -1,9 +1,11 @@
 subprojects {
 group = "scalardb.dataloader"

+


Unnecessary empty line?

brfrn169 · 2024-07-09T01:30:05Z

data-loader/build.gradle

@@ -23,5 +25,12 @@ subprojects {
 testImplementation "org.mockito:mockito-core:${mockitoVersion}"
 testImplementation "org.mockito:mockito-inline:${mockitoVersion}"
 testImplementation "org.mockito:mockito-junit-jupiter:${mockitoVersion}"
+


Ditto. Unnecessary empty line?

ypeckstadt added 5 commits May 26, 2024 10:02

Add mockito and core project in Gradle files

2e38741

WIP

3797510

Move ColumnKeyValue to data loader core project

637b474

Merge branch 'feat/data-loader/columnkeyvalue' into feat/data-loader/…

a113450

…utils

Add dataloader core key and column utils

89028b4

ypeckstadt self-assigned this May 26, 2024

ypeckstadt added the enhancement New feature or request label May 26, 2024

ypeckstadt requested review from komamitsu, brfrn169 and Torch3333 May 26, 2024 01:32

This was referenced May 26, 2024

Add TableMetadataService and utils #1772

Draft

Add connection to ScalarDB and parse keys #1768

Draft

Merge branch 'master' into feat/data-loader/utils

0c34139

komamitsu reviewed May 28, 2024

View reviewed changes

ypeckstadt added 3 commits May 29, 2024 08:09

Rename to CoreError.DATA_LOADER_INVALID_COLUMN_NON_EXISTENT

9c24445

Add @nullable to parseKeyValue

79fee4b

Pass in tableName with parseKeyValue and parsing exception

4039083

ypeckstadt requested a review from komamitsu May 28, 2024 23:21

brfrn169 reviewed May 29, 2024

View reviewed changes

Torch3333 approved these changes Jun 4, 2024

View reviewed changes

Apply error handling suggestions

2af371a

ypeckstadt requested a review from brfrn169 June 10, 2024 05:51

komamitsu reviewed Jun 10, 2024

View reviewed changes

ypeckstadt added 5 commits June 12, 2024 09:25

Remove unused CoreError

ee67ef7

Merge branch 'master' into feat/data-loader/utils

d06c3b5

Merge branch 'master' into feat/data-loader/utils

57738d3

Add Lombok to build.gradle for dataloader project

c451948

Merge branch 'master' into feat/data-loader/utils

d12526c

brfrn169 approved these changes Jul 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add data loader core key and column utils #1771

Add data loader core key and column utils #1771

ypeckstadt commented May 26, 2024 •

edited

Loading

komamitsu May 28, 2024

ypeckstadt May 28, 2024

komamitsu May 29, 2024

ypeckstadt Jun 10, 2024

komamitsu Jun 10, 2024

ypeckstadt Jul 4, 2024 •

edited

Loading

brfrn169 left a comment

brfrn169 May 29, 2024

ypeckstadt Jun 10, 2024

brfrn169 May 29, 2024

ypeckstadt Jun 10, 2024

brfrn169 May 29, 2024

Torch3333 left a comment

komamitsu Jun 10, 2024

ypeckstadt Jun 12, 2024

brfrn169 left a comment

brfrn169 Jul 9, 2024

brfrn169 Jul 9, 2024 •

edited

Loading

	public static Column<?> createColumnFromValue(DataType dataType, String columnName, String value)
	public static Column<?> createColumnFromValue(DataType dataType, String columnName, ＠Nullable String value)

Add data loader core key and column utils #1771

Are you sure you want to change the base?

Add data loader core key and column utils #1771

Conversation

ypeckstadt commented May 26, 2024 • edited 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypeckstadt Jul 4, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torch3333 left a comment

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

brfrn169 Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

ypeckstadt commented May 26, 2024 •

edited

Loading

ypeckstadt Jul 4, 2024 •

edited

Loading

brfrn169 Jul 9, 2024 •

edited

Loading