Add Util classes for data loader #2388

inv-jishnu · 2024-12-04T11:08:01Z

Description

Added util classes for data loader

Related issues and/or PRs

NA

Changes made

Added a few util classes used in data loader.
Also includes a helper file for unit test and an file with error messages.

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)

This PR is part of the on-going process of merging all ScalarDB Data Loader code into the main repository.

Release notes

NA

Torch3333

LGTM, thank you!

komamitsu · 2024-12-12T02:15:26Z

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

+   * @return formatted double as a string
+   */
+  public static String convertToNonScientific(Double doubleValue) {
+    return createFormatter().format(doubleValue);


If this method is frequently called, I think the DecimalFormat should be reused.

DecimalFormat DECIMAL_FORMAT = createFormatter(); ... public static String convertToNonScientific(Double doubleValue) { return DECIMAL_FORMAT.format(doubleValue); }

@komamitsu san, I had added this change but had to revert back based on Suzuki-san's comment.

Ah, it's used by multiple threads, right. Understood. Using ThreadLocal is might be an option, though.

komamitsu · 2024-12-12T02:19:14Z

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

+      return "";
+    }
+
+    if (!path.endsWith("/")) {


[minor] If this tool could be used on Windows environment, java.io.File#separatorChar should be used instead.

I have updated the code to use the java.io.File#separatorChar.
Thank you.

brfrn169

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

brfrn169 · 2024-12-12T14:37:08Z

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

+
+public class DebugUtil {
+
+  private static final Logger LOGGER = LoggerFactory.getLogger(DebugUtil.class);


Please use lower camel case for the logger:

Suggested change

private static final Logger LOGGER = LoggerFactory.getLogger(DebugUtil.class);

private static final Logger logger = LoggerFactory.getLogger(DebugUtil.class);

According to the Google Java Style Guide, loggers are not considered constants: https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names

I have updated the name to logger.
Thank you.

brfrn169 · 2024-12-12T14:41:45Z

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

+/** Utils for decimal handling */
+public class DecimalUtil {
+
+  private static final DecimalFormat DECIMAL_FORMAT = createFormatter();


DecimalFormat isn't thread-safe:
https://www.baeldung.com/java-decimalformat#thread-safety

We should not share the same instance between threads.

@brfrn169 san,

I had changed this based on an earlier comment. I have reverted this back.

brfrn169 · 2024-12-12T14:43:58Z

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

+    int N = collections[0].size();
+    for (Collection<?> a : collections) if (a.size() != N) return false;
+    return true;


Suggested change

int N = collections[0].size();

for (Collection<?> a : collections) if (a.size() != N) return false;

return true;

int n = collections[0].size();

for (Collection<?> c : collections) {

if (c.size() != n) {

return false;

}

}

return true;

I have updated the change as suggested.
Thank you.

brfrn169 · 2024-12-12T14:44:56Z

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

+    for (Object value : values) {
+      if (value == null) {
+        throw new NullPointerException(DATA_LOADER_ERROR_METHOD_NULL_ARGUMENT.getMessage());


Suggested change

for (Object value : values) {

if (value == null) {

throw new NullPointerException(DATA_LOADER_ERROR_METHOD_NULL_ARGUMENT.getMessage());

for (Object value : values) {

if (value == null) {

throw new NullPointerException(DATA_LOADER_ERROR_METHOD_NULL_ARGUMENT.buildMessage());

I have updated the change as suggested.
Thank you.

inv-jishnu · 2024-12-13T05:55:51Z

@komamitsu san, @brfrn169 san,
I have updated the PR based on your feedback.
Please take a look at this again when you get a chance.
Thank you.

komamitsu

LGTM! 👍

feeblefakie

LGTM! Thank you!

brfrn169 · 2024-12-16T05:23:27Z

data-loader/core/src/test/java/com/scalar/db/dataloader/core/util/RuntimeUtilTest.java

+public class RuntimeUtilTest {
+
+  @Test
+  public void checkNotNull_HasNullValues_ShouldThrowException() {
+    assertThatThrownBy(() -> RuntimeUtil.checkNotNull(null, null))
+        .isExactlyInstanceOf(NullPointerException.class)
+        .hasMessage(DATA_LOADER_ERROR_METHOD_NULL_ARGUMENT.buildMessage());
+  }
+
+  @Test
+  public void checkNotNull_HasNoNullValues_ShouldNotThrowException() {
+    String string = "1";
+    Object object = new Object();
+    RuntimeUtil.checkNotNull(string, object);
+  }
+}


For consistency:

Suggested change

public class RuntimeUtilTest {

@Test

public void checkNotNull_HasNullValues_ShouldThrowException() {

assertThatThrownBy(() -> RuntimeUtil.checkNotNull(null, null))

.isExactlyInstanceOf(NullPointerException.class)

.hasMessage(DATA_LOADER_ERROR_METHOD_NULL_ARGUMENT.buildMessage());

}

@Test

public void checkNotNull_HasNoNullValues_ShouldNotThrowException() {

String string = "1";

Object object = new Object();

RuntimeUtil.checkNotNull(string, object);

}

}

class RuntimeUtilTest {

@Test

void checkNotNull_HasNullValues_ShouldThrowException() {

assertThatThrownBy(() -> RuntimeUtil.checkNotNull(null, null))

.isExactlyInstanceOf(NullPointerException.class)

.hasMessage(DATA_LOADER_ERROR_METHOD_NULL_ARGUMENT.buildMessage());

}

@Test

void checkNotNull_HasNoNullValues_ShouldNotThrowException() {

String string = "1";

Object object = new Object();

RuntimeUtil.checkNotNull(string, object);

}

}

Thank you.
I have updated it as suggested to make it consistent.

brfrn169 · 2024-12-16T05:29:54Z

data-loader/build.gradle

@@ -1,4 +1,8 @@
 subprojects {
+


Suggested change

I have removed the empty line.
Thank you

Util classes for data loader

753618b

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

inv-jishnu marked this pull request as draft December 4, 2024 11:08

inv-jishnu added 4 commits December 4, 2024 17:55

Fix spotbug issue

8d39d02

Removed error message and added core error

bf94c49

Applied spotless

47be388

Fixed unit test failures

913eb1c

ypeckstadt marked this pull request as ready for review December 11, 2024 09:41

ypeckstadt self-assigned this Dec 11, 2024

ypeckstadt requested review from brfrn169, komamitsu and Torch3333 December 11, 2024 09:42

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

1f204b8

inv-jishnu mentioned this pull request Dec 11, 2024

Export options and basic export report dto #2397

Draft

6 tasks

Torch3333 approved these changes Dec 12, 2024

View reviewed changes

komamitsu reviewed Dec 12, 2024

View reviewed changes

Added DECIMAL_FORMAT

67f2474

feeblefakie self-requested a review December 12, 2024 06:49

Path util class updated

14e3593

brfrn169 reviewed Dec 12, 2024

View reviewed changes

inv-jishnu and others added 2 commits December 13, 2024 11:10

Feedback changes

a096d51

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

dbf1940

inv-jishnu requested review from brfrn169 and komamitsu December 13, 2024 05:54

komamitsu approved these changes Dec 16, 2024

View reviewed changes

feeblefakie approved these changes Dec 16, 2024

View reviewed changes

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

cd8add9

brfrn169 reviewed Dec 16, 2024

View reviewed changes

Changes

52890c8

inv-jishnu requested a review from brfrn169 December 16, 2024 06:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Util classes for data loader #2388

Add Util classes for data loader #2388

inv-jishnu commented Dec 4, 2024 •

edited by ypeckstadt

Loading

Torch3333 left a comment

komamitsu Dec 12, 2024

inv-jishnu Dec 13, 2024

komamitsu Dec 16, 2024

komamitsu Dec 12, 2024

inv-jishnu Dec 13, 2024

brfrn169 left a comment

brfrn169 Dec 12, 2024

inv-jishnu Dec 13, 2024

brfrn169 Dec 12, 2024

inv-jishnu Dec 13, 2024

brfrn169 Dec 12, 2024 •

edited

Loading

inv-jishnu Dec 13, 2024

brfrn169 Dec 12, 2024

inv-jishnu Dec 13, 2024

inv-jishnu commented Dec 13, 2024

komamitsu left a comment

feeblefakie left a comment

brfrn169 Dec 16, 2024

inv-jishnu Dec 16, 2024

brfrn169 Dec 16, 2024

inv-jishnu Dec 16, 2024


		public class DebugUtil {

		private static final Logger LOGGER = LoggerFactory.getLogger(DebugUtil.class);

Add Util classes for data loader #2388

Are you sure you want to change the base?

Add Util classes for data loader #2388

Conversation

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

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

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

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

Choose a reason for hiding this comment

brfrn169 Dec 12, 2024 • edited Loading

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 Dec 13, 2024

komamitsu left a comment

Choose a reason for hiding this comment

feeblefakie 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

inv-jishnu commented Dec 4, 2024 •

edited by ypeckstadt

Loading

brfrn169 Dec 12, 2024 •

edited

Loading