Skip to content

Conversation

thongdk8
Copy link
Contributor

@thongdk8 thongdk8 commented Jun 3, 2025

Description

This PR adds validation for import/export integer params to make sure they are positive. Intentionally, we don't allow data chunk size to be set to 0, which means the file is loaded in memory before importing as the current implementation. PTAL. Thank you.

Related issues and/or PRs

NA

Changes made

  • Add validation for import/export integer params to make sure they are positive

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)

NA

Release notes

NA

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds runtime checks to ensure that integer parameters for import/export commands are strictly positive.

  • Introduces validatePositiveValue utility and updates import/export commands to use it.
  • Adds corresponding tests in CommandLineInputUtilsTest.
  • Defines new error codes in CoreError for each validation failure.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java Added validatePositiveValue method and Javadoc
data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java Added tests covering positive, zero, and negative cases
data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataimport/ImportCommand.java Hooked up positive-value checks for import options
data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java Hooked up positive-value checks for export options
core/src/main/java/com/scalar/db/common/error/CoreError.java Added new error codes for invalid parameter values
Comments suppressed due to low confidence (2)

data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java:57

  • The Javadoc for validatePositiveValue is missing a @param error tag and the closing tag * @*/ is incorrect. Replace * @*/ with */ and add @param error the CoreError to report when the value is not positive.
* @*/

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

  • All four new error constants share the same code "0205", which can cause collisions in error handling. Assign unique codes for each distinct error.
DATA_LOADER_INVALID_DATA_CHUNK_SIZE(

@thongdk8 thongdk8 force-pushed the data-loader/imprv/add-params-validation branch from d48368a to 84c6544 Compare June 3, 2025 07:40
@thongdk8 thongdk8 marked this pull request as draft June 3, 2025 07:48
@thongdk8 thongdk8 force-pushed the data-loader/imprv/add-params-validation branch from 84c6544 to d549568 Compare June 3, 2025 07:59
@thongdk8 thongdk8 marked this pull request as ready for review June 3, 2025 08:28
Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a minor comment. Other than that, LGTM! Thank you!

Copy link
Contributor

@inv-jishnu inv-jishnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thank you.

Copy link
Contributor

@ypeckstadt ypeckstadt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@brfrn169 brfrn169 merged commit b61607a into master Jun 9, 2025
55 checks passed
@brfrn169 brfrn169 deleted the data-loader/imprv/add-params-validation branch June 9, 2025 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants