Enhancing the SQL Interval syntax: A story of Open Source contribution
Tags: rust, beginners, opensource, sql
Published: 2024-07-09 21:54:11


There are many reasons why developers dive into the world of Open Source contributions.
 
Contributing can be a way to give back to the community and use your skills for the greater good. It's a fantastic environment that allows you to network with talented developers, build relationships, and potentially find mentors or collaborators. For those seeking career advancement, contributions become a public portfolio showcasing your skills and experience. Sometimes, it's about a personal itch! You might encounter a bug or limitation in a project you rely on, and contributing a fix not only solves your frustration but benefits everyone who uses that software.

The recipe for a successful Open Source contribution goes beyond just code. A strong desire to learn fuels the journey, as you navigate unfamiliar codebases and tackle new challenges. But learning flourishes best within a supportive environment. 

A responsive community acts as a safety net, offering guidance and feedback to help you refine your contributions. Ideally, you can also "dogfood" the tool you contribute to, using it in your work or personal projects. This firsthand experience provides valuable context for your contributions and ensures they address real-world needs. With these elements in place, you're well on your way to making a lasting impact on the Open Source community.

While I've been building my software development skills for a while now,  Rust still feels like a whole new world to explore. This “newbie” feeling might make some shy away from contribution, fearing their code won't be good enough. I use silly mistakes as stepping stones to improve my skills, not a reason to feel discouraged. 

Over a year of contributing to [GreptimeDB](https://github.com/GreptimeTeam/greptimedb) has been a rewarding journey filled with learning experiences. Today, I'd like to walk you through one of those. Let's get our hands dirty (or should I say, claws? 🦀)
This time we will be enhancing the Interval syntax by allowing the [shortened version]

## Motivation
This time I chose [a task](https://github.com/GreptimeTeam/greptimedb/issues/4168) to enhance the Interval syntax by allowing a shortened version. The SQL standard specifies a syntax format such as:
```sql
select interval '1 hour';
```
My objective was to ensure the functionality of the following alternative(_shortened_) format:
```sql
select interval '1h';
```

Diving right into the code, I discovered that the core functionality for handling transformations already exists. The scope of a change boils down to adding a new rule specifically for the [Interval](https://docs.rs/sqlparser/latest/sqlparser/ast/struct.Interval.html) data type: intervals with abbreviated time formats will be automatically expanded to their full versions. Let's take a closer look at a specific section of the code that does the logic

```rust
fn visit_expr(&self, expr: &mut Expr) -> ControlFlow<()> {
    match expr {
        Expr::Interval(interval) => match *interval.value.clone() {
            Expr::Value(Value::SingleQuotedString(value)) => {
                if let Some(data) = expand_interval_name(&value) {
                    *expr = create_interval_with_expanded_name(
                        interval,
                        single_quoted_string_expr(data),
                    );
                }
            }
            Expr::BinaryOp { left, op, right } => match *left {
                Expr::Value(Value::SingleQuotedString(value))=> {
                    if let Some(data) = expand_interval_name(&value) {
                        let new_value = Box::new(Expr::BinaryOp {
                            left: single_quoted_string_expr(data),
                            op,
                            right,
                        });
                        *expr = create_interval_with_expanded_name(interval, new_value);
                    }
............
}
```
## Code Review
An experienced Rust developer, [Dennis](https://x.com/killme20082) (CEO & Co-Founder @ Greptime), quickly identified an area for improvement and [suggested the fix](https://github.com/GreptimeTeam/greptimedb/pull/4220#discussion_r1655250624):

![Dennis spotted an improvement opportunity](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/w28r729u6k44uin5inry.png)
Code review shines as a learning tool. 

Beyond simply accepting the suggested change (though the rationale for “efficiency” was clear!), I decided to take a deep dive. Analyzing the proposed improvement and explaining it to myself(in the form of this post), helped me better understand the Rust code and its recommended practices.

### Avoiding unnecessary cloning and ownership transfers
Originally I used `.clone()` on `interval.value`:
```rust
match *interval.value.clone() { ... }
```

Cloning here creates a new instance of the data each time, which can be inefficient if the data structure is large or cloning is expensive. The suggested version avoids this by using references:

```rust
match &*interval.value { ... }
```

Matching on a reference `(&*interval.value)` eliminates the cost of cloning. The same improvement is applied to the match on `left` in the binary operation case:

```rust
match &**left { ... }
```

This one is slightly more involved: it uses a double dereference to get a reference to the value inside a `Box`, which is more efficient than cloning.

### Clearer Pattern Matching

Using references in pattern matching emphasizes the intention of only reading data, not transferring ownership:

```rust
match &*interval.value { ... }
```

This shows explicitly that the matched value is not being moved. It helps with reasoning about the code, especially in a context with complex ownership and borrowing rules.

### Reduced Cloning Inside the Binary Operation

In the original code, the `op` and `right` fields of the `Expr::BinaryOp` variant are cloned unconditionally.

```rust
let new_value = Box::new(Expr::BinaryOp {
    left: single_quoted_string_expr(data),
    op,
    right,
});
```

However, they only need to be cloned if the `left` field is an `Expr::Value` variant with a string value. The suggested enhancement moves the cloning inside the `if let` block, so it only happens when necessary.

```rust
let new_value = Box::new(Expr::BinaryOp {
    left: single_quoted_string_expr(data),
    op: op.clone(),
    right: right.clone(),
});
```

### Using references instead of cloning: 
In the original code, `expand_interval_name(&value)` is used, which borrows value. ~~However, value is of type `String`, which implements the `AsRef<str>` trait. This means that it can be automatically dereferenced to a `&str` when necessary~~. However, value is of type `String`, which implements the  `Deref<Target=str>` trait (more information could be found [here](https://doc.rust-lang.org/std/ops/trait.Deref.html#deref-coercion)). This means that it can be automatically dereferenced to a `&str` when necessary. The improved version uses `expand_interval_name(value)`, which avoids the need for explicit reference.

## Summary
So in the context of this change “efficiency” stands for: 
• Avoiding unnecessary cloning, reducing overhead.
• Making the borrowing and ownership patterns clearer and safer.
• Enhancing overall readability and maintainability.

This is how the `visit_expr` function looks like after all suggestions have been applied: 

```rust
fn visit_expr(&self, expr: &mut Expr) -> ControlFlow<()> {
        match expr {
            Expr::Interval(interval) => match &*interval.value {
                Expr::Value(Value::SingleQuotedString(value))
                | Expr::Value(Value::DoubleQuotedString(value)) => {
                    if let Some(expanded_name) = expand_interval_name(value) {
                        *expr = update_existing_interval_with_value(
                            interval,
                            single_quoted_string_expr(expanded_name),
                        );
                    }
                }
                Expr::BinaryOp { left, op, right } => match &**left {
                    Expr::Value(Value::SingleQuotedString(value))
                    | Expr::Value(Value::DoubleQuotedString(value)) => {
                        if let Some(expanded_name) = expand_interval_name(value) {
                            let new_expr_value = Box::new(Expr::BinaryOp {
                                left: single_quoted_string_expr(expanded_name),
                                op: op.clone(),
                                right: right.clone(),
                            });
                            *expr = update_existing_interval_with_value(interval, new_expr_value);
                        }
                    }
                    _ => {}
                },
    .....................
    }
```

Open Source contribution has been an incredible way to accelerate my Rust learning curve. My efforts on this **#GoodFirstIssue** serve as an illustration of how to improve skills through collaborations. Depending on your feedback, I'm excited to share more of these learning experiences in future posts!

A huge thanks to the entire [Greptime](https://github.com/GreptimeTeam/greptimedb) team, especially [Dennis](https://x.com/killme20082), for their support and guidance! Let’s keep the contributing/learning going!

**UPD**: Thanks to @tisonkun for the thorough review!