Split schema-loader errors and data-loader errors from CoreError #2805

brfrn169 · 2025-06-20T05:22:14Z

Description

This PR splits the schema-loader errors and the data-loader errors from CoreError.

Related issues and/or PRs

N/A

Changes made

Added SchemaLoderError, and moved the schema-loader related errors to it from CoreError.
Added DataLoderError, and moved the data-loader related errors to it CoreError.
Move CoreError to the package com.scalar.db.common.

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.
I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
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)

N/A

Release notes

N/A

Copilot

Pull Request Overview

This PR extracts schema-loader and data-loader errors from CoreError into their own enums (SchemaLoaderError, DataLoaderError) and updates all references accordingly.

Introduced SchemaLoaderError enum and replaced CoreError usage in schema-loader module
Introduced DataLoaderError enum and updated tests and code in data-loader and CLI modules to use it
Added uniqueness tests for both error enums

Reviewed Changes

Copilot reviewed 45 out of 45 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
schema-loader/src/main/java/com/scalar/db/schemaloader/SchemaLoaderError.java	New enum for schema-loader error codes
schema-loader/src/main/java/com/scalar/db/schemaloader/command/SchemaLoaderCommand.java	Replaced CoreError with SchemaLoaderError in CLI commands
schema-loader/src/main/java/com/scalar/db/schemaloader/alteration/TableMetadataAlterationProcessor.java	Updated error references to use SchemaLoaderError
schema-loader/src/main/java/com/scalar/db/schemaloader/TableSchema.java	Swapped CoreError for SchemaLoaderError in parsing logic
schema-loader/src/main/java/com/scalar/db/schemaloader/SchemaOperator.java	Updated table-not-found error to SchemaLoaderError
schema-loader/src/test/java/com/scalar/db/schemaloader/SchemaLoaderErrorTest.java	Added duplicate-code check for SchemaLoaderError
data-loader/core/src/main/java/com/scalar/db/dataloader/core/DataLoaderError.java	New enum for data-loader error codes
data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/*.java	Replaced CoreError with DataLoaderError in utility classes
data-loader/core/src/test/java/com/scalar/db/dataloader/core/*.java	Updated tests to use DataLoaderError
data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/*.java	Replaced CoreError with DataLoaderError in CLI utils
data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/*.java	Updated CLI tests to use DataLoaderError

Comments suppressed due to low confidence (2)

schema-loader/src/main/java/com/scalar/db/schemaloader/command/SchemaLoaderCommand.java:144

Consider throwing SchemaLoaderException (or wrapping the IllegalArgumentException) here instead of a raw IllegalArgumentException, to align with the method signature and centralize error handling.

      throw new IllegalArgumentException(

schema-loader/src/main/java/com/scalar/db/schemaloader/SchemaLoaderError.java:1

[nitpick] You may want to add class-level Javadoc to SchemaLoaderError explaining its purpose and how error codes map to user scenarios.

package com.scalar.db.schemaloader;

Copilot · 2025-06-20T05:24:24Z

schema-loader/src/main/java/com/scalar/db/schemaloader/SchemaLoaderError.java

+  SPECIFYING_SCHEMA_FILE_REQUIRED_WHEN_USING_ALTER(
+      Category.USER_ERROR,
+      "0007",
+      "Specifying the '--schema-file' option is required when using the '--alter' option",


[nitpick] For consistency with other messages, consider adding a trailing period to this error message.

Suggested change

"Specifying the '--schema-file' option is required when using the '--alter' option",

"Specifying the '--schema-file' option is required when using the '--alter' option.",

Odd that Copilot suggests the period only here. If we apply the period here, we should apply it consistently to all other messages. However, I believe it was decided that we wouldn't add periods to messages that contained only one sentence.

brfrn169 · 2025-06-20T05:33:39Z

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

+import com.scalar.db.common.error.Category;
+import com.scalar.db.common.error.ScalarDbError;
+
+public enum DataLoaderError implements ScalarDbError {


@josh-wong IIRC, you haven’t checked the error messages in data-loader yet. If that’s correct, could you please take a look? 🙇

brfrn169 · 2025-06-20T05:39:30Z

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

+      Category.INTERNAL_ERROR, "0006", "Failed to read JSON Lines file. Details: %s", "", ""),
+  ;
+
+  private static final String COMPONENT_NAME = "DB-DATA-LOADER";


@feeblefakie The prefix of the error code for data-loader is DB-DATA-LOADER. Is that okay?

brfrn169 · 2025-06-20T05:40:30Z

schema-loader/src/main/java/com/scalar/db/schemaloader/SchemaLoaderError.java

+      Category.USER_ERROR, "0017", "Invalid column type. Table: %s; Column: %s; Type: %s", "", ""),
+  ;
+
+  private static final String COMPONENT_NAME = "DB-SCHEMA-LOADER";


@feeblefakie The prefix of the error code for schema-loader is DB-SCHEMA-LOADER. Is that okay?

komamitsu

LGTM! 👍

feeblefakie

LGTM! Thank you!

ypeckstadt

LGTM. Thank you.

inv-jishnu

LGTM!
Thank you!

thongdk8

LGTM! Thank you!

josh-wong

I've added some comments and suggestions. PTAL!

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

schema-loader/src/main/java/com/scalar/db/schemaloader/SchemaLoaderError.java

josh-wong · 2025-06-20T06:44:22Z

schema-loader/src/main/java/com/scalar/db/schemaloader/SchemaLoaderError.java

+  SPECIFYING_SCHEMA_FILE_REQUIRED_WHEN_USING_ALTER(
+      Category.USER_ERROR,
+      "0007",
+      "Specifying the '--schema-file' option is required when using the '--alter' option",


Odd that Copilot suggests the period only here. If we apply the period here, we should apply it consistently to all other messages. However, I believe it was decided that we wouldn't add periods to messages that contained only one sentence.

Torch3333

LGTM, thank you!

Co-authored-by: Josh Wong <[email protected]>

brfrn169 · 2025-06-20T07:01:03Z

@josh-wong I've just applied your suggestions. Could you please dobule-check? 🙇

josh-wong

LGTM! Thank you!🙇🏻‍♂️

…errors-from-CoreError

Co-authored-by: Josh Wong <[email protected]>

brfrn169 self-assigned this Jun 20, 2025

brfrn169 added the improvement label Jun 20, 2025

brfrn169 requested a review from Copilot June 20, 2025 05:22

Split schema-loader errors and data-loader errors from CoreError

9902070

Copilot AI reviewed Jun 20, 2025

View reviewed changes

brfrn169 force-pushed the split-schema-loder-errors-and-data-loader-errors-from-CoreError branch from f2400a0 to 9902070 Compare June 20, 2025 05:28

brfrn169 requested review from Torch3333, feeblefakie, inv-jishnu, josh-wong, komamitsu, thongdk8 and ypeckstadt June 20, 2025 05:31

brfrn169 commented Jun 20, 2025

View reviewed changes

komamitsu approved these changes Jun 20, 2025

View reviewed changes

feeblefakie approved these changes Jun 20, 2025

View reviewed changes

ypeckstadt approved these changes Jun 20, 2025

View reviewed changes

inv-jishnu approved these changes Jun 20, 2025

View reviewed changes

thongdk8 approved these changes Jun 20, 2025

View reviewed changes

josh-wong requested changes Jun 20, 2025

View reviewed changes

Torch3333 approved these changes Jun 20, 2025

View reviewed changes

[skip ci] Apply suggestions from code review

5679f61

Co-authored-by: Josh Wong <[email protected]>

brfrn169 requested a review from josh-wong June 20, 2025 06:58

[skip ci] Apply suggestions from code review

c1e9ab0

Co-authored-by: Josh Wong <[email protected]>

josh-wong approved these changes Jun 20, 2025

View reviewed changes

brfrn169 added 2 commits June 20, 2025 16:03

Merge branch 'master' into split-schema-loder-errors-and-data-loader-…

3700267

…errors-from-CoreError

Fix

8188d24

brfrn169 merged commit de96782 into master Jun 20, 2025
106 of 108 checks passed

brfrn169 deleted the split-schema-loder-errors-and-data-loader-errors-from-CoreError branch June 20, 2025 08:41

feeblefakie mentioned this pull request Jun 20, 2025

Backport to branch(3) : Split schema-loader errors and data-loader errors from CoreError #2809

Merged

brfrn169 added a commit that referenced this pull request Jun 20, 2025

Split schema-loader errors and data-loader errors from CoreError (#2805)

5b723ed

Co-authored-by: Josh Wong <[email protected]>

	"Specifying the '--schema-file' option is required when using the '--alter' option",
	"Specifying the '--schema-file' option is required when using the '--alter' option.",

Split schema-loader errors and data-loader errors from CoreError #2805

Split schema-loader errors and data-loader errors from CoreError #2805

Uh oh!

Conversation

brfrn169 commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

josh-wong Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

komamitsu left a comment

Choose a reason for hiding this comment

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

ypeckstadt left a comment

Choose a reason for hiding this comment

Uh oh!

inv-jishnu left a comment

Choose a reason for hiding this comment

Uh oh!

thongdk8 left a comment

Choose a reason for hiding this comment

Uh oh!

josh-wong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josh-wong Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

Torch3333 left a comment

Choose a reason for hiding this comment

Uh oh!

brfrn169 commented Jun 20, 2025

Uh oh!

josh-wong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

brfrn169 commented Jun 20, 2025 •

edited

Loading