Skip to content

Add support for the Duration CQL data type #53

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 27, 2025
Merged

Conversation

vponomaryov
Copy link
Collaborator

Example of the Duration CQL data type usage in a rune script:

  let d1 = "1y3mo2w6d13h14m22s33ms44us55ns";
  let d2 = "55mo345d11ns";
  let d3 = "1m5s3ms";
  let d4 = "5s";

@vponomaryov vponomaryov requested a review from Copilot March 26, 2025 10:33
Copy link

@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 support for the Duration CQL data type by introducing a new branch in the value conversion function that parses duration strings into a CqlDuration.

  • Added a new match arm for Value::String(s) handling for NativeType::Duration.
  • Introduced regex-based parsing to extract duration components from ISO 8601–like strings.
Comments suppressed due to low confidence (2)

src/scripting/bind.rs:107

  • Consider adding unit tests for the new duration parsing logic to cover a variety of valid and invalid ISO 8601 format examples.
(Value::String(s), ColumnType::Native(NativeType::Duration)) => {

src/scripting/bind.rs:159

  • [nitpick] The error message 'Got invalid duration value' could include additional context (such as which part of the input was unrecognized) to aid in debugging.
} else if cap.name("invalid").is_some() {

@vponomaryov
Copy link
Collaborator Author

vponomaryov commented Mar 26, 2025

@fruch , @soyacz
I would like to have some review here because of the data transformation specifics.

To try out it locally do following:

$ make docker-build

Copy link

@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 support for the CQL Duration data type by introducing a new parsing branch in the value conversion function. Key changes include:

  • Adding a new regex-based duration parser in src/scripting/bind.rs.
  • Extending the match in to_scylla_value to handle NativeType::Duration.
  • Updating Cargo.toml to include the once_cell dependency.

Reviewed Changes

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

File Description
src/scripting/bind.rs Implements duration parsing using a regex and converts it to CqlDuration.
Cargo.toml Adds the once_cell dependency required for lazy regex initialization.
Comments suppressed due to low confidence (2)

src/scripting/bind.rs:19

  • Consider anchoring the duration regex with '^' and '$' to ensure that the entire input string is validated against the expected duration format.
static DURATION_REGEX: Lazy<Regex> = Lazy::new(|| {

src/scripting/bind.rs:186

  • [nitpick] Consider providing additional details in the error message about the supported duration format to aid debugging.
else if cap.name("invalid").is_some() {

Copy link

@soyacz soyacz left a comment

Choose a reason for hiding this comment

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

maybe worth to add unit tests for that?
Otherwise, if performance is key, there's nom library that could help in that scope

Example of the Duration CQL data type usage in a rune script:

  let d1 = "1y3mo2w6d13h14m22s33ms44us55ns";
  let d2 = "55mo345d11ns";
  let d3 = "1m5s3ms";
  let d4 = "5s";
@vponomaryov
Copy link
Collaborator Author

@soyacz
Added unit tests

@vponomaryov vponomaryov merged commit 8703b11 into main Mar 27, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants