Allow an optional unit when specifying bytes in SQL

[Sigurd00]

This PR closes #407 by adding support for an optional unit (B, KB, MB, GB, TB) after num_bytes in [CLUSTER] [table_name[, table_name]+] [TARGET num_bytes[unit]], e.g. TARGET 1024 MB is automatically converted to bytes.

Unit testing have also been added in parser.rs covering each unit, case and the relevant error cases.

This is my first PR on the project, so let me know if I've missed anything terms of conventions or process.

[CGodiksen]

Thank you for the well-written PR. My apologies for the late review but we have been on summer vacation for the last 3 weeks. Regarding the PR, everything looks good and follows the conventions of the repository. The check currently fails due to cargo clippy but that will be fixed in #412 so don't worry about that in this PR.

The syntax you added looks good, I just want one extra test to ensure that the space between the number and the unit is actually optional (which is what we want). Thank you again for your interest in our project!

[Copilot]

🟡 Not ready to approve

Unsupported units currently fall through to a generic “expected end of statement” error despite documentation implying explicit unit validation, so error handling/messages should be made consistent and user-focused.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Adds support in the ModelarDB SQL parser for specifying an optional byte unit (B/KB/MB/GB/TB) after TARGET num_bytes in OPTIMIZE, converting the value to bytes during parsing to improve ergonomics for users.

Changes:

• Extend OPTIMIZE ... TARGET parsing to accept an optional, case-insensitive byte unit and multiply accordingly.
• Add unit parsing helpers (parse_unsigned_literal_u64_with_optional_byte_unit, byte_unit_multiplier) including overflow handling.
• Add unit-focused tests covering supported units, casing, spacing, and overflow.

File summaries

File	Description
crates/modelardb_storage/src/parser.rs	Implements optional byte-unit parsing for OPTIMIZE ... TARGET and adds unit/overflow test coverage.

Review details

Suppressed comments (2)

crates/modelardb_storage/src/parser.rs:687

• After parsing TARGET's numeric value, if the next token is a word but not a supported unit, this function currently returns Ok(value) and leaves the token to be rejected later by the generic "expected end of statement" check. This contradicts the OPTIMIZE docstring (which says unsupported units are a parse error) and produces an unhelpful user-facing error message. Consider explicitly validating and erroring on any trailing word that isn’t one of the supported units.

        let maybe_unit_and_multiplier = if let Token::Word(word) = parser.peek_nth_token(0).token {
            self.byte_unit_multiplier(&word.value)
                .map(|multiplier| (word.value, multiplier))
        } else {
            None

crates/modelardb_storage/src/parser.rs:2666

• This test currently asserts the generic sqlparser "Expected: end of statement" error for an invalid unit. If the parser is updated to explicitly validate/raise a targeted error for unsupported byte units, adjust this expected string accordingly so the test matches the improved user-facing error message.

        assert_eq!(
            result.unwrap_err().to_string(),
            "Parser Error: sql parser error: Expected: end of statement, found: XY at Line: 1, Column: 22"
        );

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[CGodiksen]

Thank you. The change suggested by Copilot also makes sense and should be applied before the PR is merged.

[skejserjensen]

LGTM, once again thank you for the PR.

[skejserjensen]

@Sigurd00 and @CGodiksen #412 have now been merged, so if the branch is rebased onto main, clippy should be happy again :)

[CGodiksen]

The PR has now been merged, thank you for your contribution @Sigurd00!