Implement styles by ddimaria · Pull Request #538 · tafia/calamine

ddimaria · 2025-07-24T21:32:14Z

Add style support for Calamine.

Basic api for getting styles

fn test_worksheet_style_iter() {
    let mut xlsx: Xlsx<_> = wb("styles.xlsx");
    let styles = xlsx.worksheet_style("Sheet 1").unwrap();

    for (row, styles) in styles.rows().enumerate() {
        for (col, style) in styles.iter().enumerate() {
            println!("row: {}, col: {}, style: {:?}", row, col, style);
        }
    }
}

Here is the relevant structs so far:

struct Style {
    font: Option<Font>,
    fill: Option<Fill>,
    borders: Option<Borders>,
    alignment: Option<Alignment>,
    number_format: Option<NumberFormat>,
    protection: Option<Protection>,
    style_id: Option<u32>,
}

struct Color {
    alpha: u8,
    red: u8,
    green: u8,
    blue: u8,
}

struct Borders {
    left: Border,
    right: Border,
    top: Border,
    bottom: Border,
    diagonal_down: Border,
    diagonal_up: Border,
}

struct Font {
    name: Option<String>,
    size: Option<f64>,
    weight: FontWeight,
    style: FontStyle,
    underline: UnderlineStyle,
    strikethrough: bool,
    color: Option<Color>,
    family: Option<String>,
}

struct Alignment {
    horizontal: HorizontalAlignment,
    vertical: VerticalAlignment,
    text_rotation: TextRotation,
    wrap_text: bool,
    indent: Option<u8>,
    shrink_to_fit: bool,
}

struct Fill {
    pattern: FillPattern,
    foreground_color: Option<Color>,
    background_color: Option<Color>,
}

struct NumberFormat {
    format_code: String,
    format_id: Option<u32>,
}

struct Protection {
    locked: bool,
    hidden: bool,
}

jmcnamara · 2025-07-26T10:51:49Z

Just a quick note to say that overall this looks good and I think it is going in the right direction.

I'll give more detailed feedback early next week.

For code comments you are already using correct sentence-case but could you also add a full stop/period at the end of each line. This makes it easier to change comments into documentation. It isn't a rule that has been adhered to in the existing code but I would like to improve that going forward.

azarmadr · 2025-07-29T13:07:55Z

Man, i wish i discovered this pull early, i wanted to know if a cell is stricken or not. Sat whole day and edited the code myself. Came till adding if stricken or not from the styles.

Learnt a lot from my experience though.
i'll test this pull against my code.

azarmadr · 2025-07-30T16:44:22Z

Is the data and styles coming together?
If not, then would we have to read the excel twice to get data, once and then style?

ddimaria · 2025-07-30T16:53:24Z

Is the data and styles coming together? If not, then would we have to read the excel twice to get data, once and then style?

Data and Formats have always been processed separately (e.g. worksheet_range() and worksheet_formula()). This PR follows the format pattern (worksheet_style() for styles and worksheet_layout() for column widths and row heights).

When I finish the base work, I'm going to look into optimizations:

More optimal storage (Vec vs Run-Length Encoding) for styles
Looping (to your point, combining read operations to reduce memory/cpu usage)

azarmadr · 2025-07-31T05:09:02Z

i got it, even formulae were being used similarly. Was able to satisfy my requirement with worksheet_sytle over a range.

Another doubt, why every item in the style is an option?
All the fonts, borders, and others are available as ids in cellxfs?

hope i am not bothering you.😊

ddimaria · 2025-08-13T17:39:31Z

Another doubt, why every item in the style is an option?

@azarmadr this is because the particular style may not exist for a given cell.

ddimaria · 2025-08-13T17:41:31Z

@jmcnamara We're currently using this fork in production and it's working well. What do I need to do to get this merged with Calamine? Some things to discuss:

Is the API agreeable?
What type of documentation is needed?
Add more testing?
Should this be a compiler feature flag?
This is only implemented for xlsx, is that OK?

jmcnamara · 2025-08-14T16:14:52Z

What do I need to do to get this merged with Calamine?

You should create a non-draft PR with all the WIP files and code removed. I will try to do a review on the current code.

Is the API agreeable?

Overall I think it is okay but I may have specific comments during review.

What type of documentation is needed?

Copy the style of the Xlsx documentation:

One line description.
One-two paragraph explanation.
Parameters section.
Errors section if the method Result<(), XlsxError>.
Examples section.

https://docs.rs/calamine/latest/calamine/struct.Xlsx.html#method.merged_regions_by_sheet

Add more testing?

That is always good. :-)

Should this be a compiler feature flag?

I think not.

This is only implemented for xlsx, is that OK?

Probably not but lets try get it right for Xlsx first.

jmcnamara · 2025-08-14T16:16:19Z

STYLE_FEATURE.md

@@ -0,0 +1,253 @@
+# Style Support in Calamine


Remove this files from the final PR or convert it to documentation.

examples/style.rs

jmcnamara · 2025-08-14T17:16:35Z

src/datatype.rs

+    /// The cell value
+    pub value: Data,
+    /// The cell style
+    pub style: Option<Style>,


Strictly speaking the style is never optional in the Excel file formats. There is always an implicit 0 style that is the default and is defined in the theme.xml file. Note, I'm not necessarily saying that it shouldn't be an Option<>.

jmcnamara · 2025-08-14T17:18:37Z

src/datatype.rs

+        self.style = None;
+    }
+
+    /// Check if the cell has style information


Again, an Excel cell will always have style information even if it is implicit for size reasons.

src/lib.rs

jmcnamara · 2025-08-14T17:21:59Z

src/xlsx/cells_reader.rs

+                    let mut style = None;
+
+                    // Extract style ID if present
+                    if let Ok(Some(style_id_str)) =


Again, the style id is 0 and not None if not present.

This is done, please review.

src/xlsx/style_parser.rs

src/style.rs

src/xlsx/style_parser.rs

src/style.rs

jmcnamara · 2025-08-14T18:16:59Z

src/style.rs

+    }
+
+    /// Get effective column width (custom or default)
+    pub fn get_effective_column_width(&self, column: u32) -> f64 {


This effective width is probably not the width that the user would expect and also may be dependent on the default font for the file.

See the following for the calculation in the other direction: https://github.com/jmcnamara/rust_xlsxwriter/blob/main/src/worksheet.rs#L19340-L19368

However, there may not be any simple way to return the correct value in all cases.

jmcnamara · 2025-08-14T18:17:31Z

src/style.rs

+    }
+
+    /// Get effective row height (custom or default)
+    pub fn get_effective_row_height(&self, row: u32) -> f64 {


Same comment for row height although the height calculation is simpler.

src/xlsx/mod.rs

test_indexed_colors.rs

jmcnamara · 2025-08-14T18:29:38Z

src/xlsx/style_parser.rs

 use crate::style::*;
 use crate::XlsxError;

+/// Get default Excel 2007+ theme color scheme (Office theme)


Avoid using /// for non-public non-doc comments. Also this is an example of where proper sentence punctuation should be applied.

tests/test.rs

jmcnamara · 2025-08-14T18:36:47Z

First review done. Some general comments:

Please squash and or rebase WIP code and reverts into a small number of logical commits or one commit.
Fix the clippy and or format warnings the are breaking the CI.
Add a full stop/period at the end of all comments.
Avoid /// comments for non-public non-doc comments. Use // instead.
Post an explanation here for the setting functions in CellData and Style.

jmcnamara · 2025-09-04T09:48:55Z

@ddimaria Any update on this?

louisdewar · 2025-11-11T21:30:25Z

src/xlsx/cells_reader.rs

+                    };
+
+                    if style_id < self.styles.len() {
+                        style = Some(self.styles[style_id].clone());


I'm not sure if this is in scope for this project, but at work we've been dealing with a lot of very large workbooks and I'm concerned about the performance effect of cloning the style for each cell (especially because the style object includes strings).

Personally I'd prefer just exposing the style ID and also the styles table and allowing users to map it as required. Although I understand the ergonomics of that are not as nice, I still think it would avoid some potential performance impact.

To be clear I'm saying this without any data to back it up, but anecdotally I made a change to how this library parses formulas and got a significant* speed improvement by avoiding extra allocations for each cell.

Here's an example from a different PR: #568 (comment)

Let me know if you have any thoughts.

@louisdewar I share your concern. A good data structure for styles is Run Length Encoding (since styles often repeat). I want to add that feature, as originally discussed here: #538 (comment), though it's been tabled in this PR by the maintainer: #538 (comment)

hough it's been tabled in this PR by the maintainer: #538 (comment)

I tabled it because "stretch goal" sounded like a nice to have. If, in fact, it is something that is required to get reasonable performance then that will need to be done before merge.

I'd like to get some perf number before implementing. We've been using this branch in production and haven't noticed perf issues.

I'm happy to run some very basic benchmarking today and show the results. I'll just comment out the clone and see what impact that has and then do some flamegraphs to see if I can see a meaningful difference.

Thanks @louisdewar! I want to do the same, time permitting, with the addition of comparing perf of style vs non-style parts of the library. It also won't take very long to implement RLE.

Sorry for the delay.

Simple benchmark:

use std::env; use calamine::{open_workbook_auto, Reader}; fn main() { let file_path = env::args().nth(1).expect("Please provide a file path"); let mut workbook = open_workbook_auto(&file_path) .expect("Failed to open workbook"); for sheet_name in workbook.sheet_names() { let range = workbook .worksheet_range(&sheet_name) .expect("Failed to read worksheet"); for row in range.rows() { for cell in row { std::hint::black_box(cell); } } } }

Before

# warmup = 2 Benchmark 1: ./target/release/examples/parse_tmp ~/Downloads/DESRI_BASE.xlsm Time (mean ± σ): 1.285 s ± 0.005 s [User: 1.206 s, System: 0.075 s] Range (min … max): 1.275 s … 1.292 s 10 runs

https://share.firefox.dev/4i0pUAo
7.2% of total extraction time is spent doing a specific style.clone().

After removing .clones

I'd imagine we would have comparible performance between this and just storing the style ID for each cell.

$ git diff diff --git a/src/xlsx/cells_reader.rs b/src/xlsx/cells_reader.rs index b1b1168..e60dc2f 100644 --- a/src/xlsx/cells_reader.rs +++ b/src/xlsx/cells_reader.rs @@ -132,7 +132,7 @@ where }; if style_id < self.styles.len() { - style = Some(self.styles[style_id].clone()); + // style = Some(self.styles[style_id].clone()); } loop { @@ -211,7 +211,7 @@ where }; if style_id < self.styles.len() { - style = Some(self.styles[style_id].clone()); + // style = Some(self.styles[style_id].clone()); } loop { @@ -352,7 +352,8 @@ where { if let Ok(style_id) = atoi_simd::parse::<usize>(style_id_str) { if style_id < self.styles.len() { - self.styles[style_id].clone() + Style::new() + // self.styles[style_id].clone() } else { Style::new() }

# warmup = 2 Benchmark 1: ./target/release/examples/parse_tmp ~/Downloads/DESRI_BASE.xlsm Time (mean ± σ): 1.103 s ± 0.005 s [User: 1.038 s, System: 0.061 s] Range (min … max): 1.093 s … 1.110 s 10 runs

https://share.firefox.dev/47WkCBo

About a 14% reduction in time.

Notes

I assume the easiest way to get this speedup is to simply store style ID in the cells and expose the style table.

I haven't profiled memory, I'm not sure what the best test of this is but I imagine this would also show a reasonable impact assuming you were storing the entire workbook in memory e.g. doing a .collect() on the cell reader.

Also this benchmark could easily be biased, I just picked a large xlsx I was using at work, other workbooks might show something different.

My laptop is quite fast (m4 pro), I can imagine the different is a lot bigger on slower servers etc...

jmcnamara · 2026-01-13T10:55:59Z

@ddimaria Any update on this. I would really like to get this upstream in the next few months.

See also: #597

louisdewar · 2026-01-13T14:12:29Z

I'm also willing to help out on this if that would be useful

jmcnamara · 2026-01-13T21:43:06Z

@louisdewar Thanks. That would be great. Could you start by rebasing the PR to the latest HEAD, fixing any issues, and creating a new PR.

Or, if you want to submit your own approach that is fine too.

louisdewar · 2026-01-14T11:36:32Z

@jmcnamara sounds good. I should have some time in the next few weeks. I think my preference would be to squash all these commits into 1 with a Co-Authored-By and make changes on top or figure out ways to make smaller incremental PRs - I'll decide once I have time to dig into this in detail.

Also it might be good to let me know if you have any thoughts about implementation / things you want to see before I start trying to tidy this up.

jmcnamara · 2026-01-14T17:35:17Z

Also it might be good to let me know if you have any thoughts about implementation / things you want to see before I start trying to tidy this up.

@louisdewar

Nothing major. I made some comments above on the original PR. My main change would be to remove the Option<> from style_id because there is always an implicit style ID of 0 = "General". I would also have similar concerns to you about the memory footprint of the proposed design. I'd agree with your comment above:

Personally I'd prefer just exposing the style ID and also the styles table and allowing users to map it as required. Although I understand the ergonomics of that are not as nice, I still think it would avoid some potential performance impact.

#538 (comment)

I don't know if you saw this comment/proposal (See also: #597) but there is now a Rust based Excel number formatting crate called ssfmt that could be plugged in so that the user could have access to a Data or DataType function like:

fn get_formatted_number(&self) -> Option<String> {}

All the style IDs and lookups could then be hidden in the background from the user.

ddimaria · 2026-01-15T16:14:26Z

@ddimaria Any update on this. I would really like to get this upstream in the next few months.

See also: #597

@jmcnamara I've been a bit busy and haven't been able to return to this. My next body of work is to improve the performance of excel imports (using Calamine), so I can handle this then. That said, if there is a pressing need and someone wants to take it over, they certainly can.

Here is my plan:

Merge in main, resolve lints
Implement RunLength Encoding and look for perf improvements
Add criterion perf tests (run before and after RLE implementation)
Clean up documentation

Let me know how you'd like to proceed. Thanks! -David

jmcnamara · 2026-01-15T17:01:02Z

Thanks.

As a first step it would be good to have a rebased PR with the CI checks passing. Maybe submit that as a new PR to supersede this one. Due to the number of comments this page often fails to load.

ddimaria · 2026-01-27T20:31:08Z

Finally back on this 😄

ddimaria · 2026-01-27T22:31:37Z

I've merged main into this branch, resolving conflicts and fixing tests. I also have a 1M cell style file being used in new benchmark tests (used criterion). I'm moving on to flamegraph and will start improvements based on the results.

ddimaria · 2026-01-28T00:26:14Z

I just implemented RLE and other optimizations. I'll close this PR and open a new one tomorrow, and will squash all of my commits. The perf is a lot better now, and there is less of a memory footprint.

ddimaria · 2026-01-30T21:52:40Z

I added support for Rich Text, which handles various formats within a single cell. This is how excel stores it:

<si>
  <r>
    <rPr>
      <b/>  <!-- bold -->
      <sz val="11"/>
      <color rgb="FF0000FF"/>
      <rFont val="Calibri"/>
    </rPr>
    <t>Bold blue text</t>
  </r>
  <r>
    <rPr>
      <i/>  <!-- italic -->
      <sz val="11"/>
      <color rgb="FFFF0000"/>
    </rPr>
    <t> italic red text</t>
  </r>
</si>

Resolves merge conflicts in Cargo.toml (rstest version) and tests/test.rs (pivot table tests vs style tests — keep both). This merge adds: - Style struct with Font/Fill/Borders/Alignment/NumberFormat/Protection - styles.xml parser (src/xlsx/style_parser.rs) - worksheet_style() API returning row×col style grid - RLE-compressed StyleRange for memory efficiency Co-Authored-By: Claude Opus 4.6 <[email protected]>

ddimaria · 2026-02-24T16:39:35Z

I noticed a bug in colors where tints weren't being handled. This is now implemented.

ddimaria · 2026-03-02T14:23:14Z

@jmcnamara I need to implement conditional formatting and validations, is it OK to add them to this PR?

jmcnamara · 2026-03-02T14:57:02Z

@ddimaria This PR is already very large. It would be best to get this stabilized first (is it suitable for review now?).

You can create other PRs that are dependent on this on. Conditional formatting and Data Validation should be separate PRs.

ddimaria · 2026-03-02T15:20:04Z

@ddimaria This PR is already very large. It would be best to get this stabilized first (is it suitable for review now?).

You can create other PRs that are dependent on this on. Conditional formatting and Data Validation should be separate PRs.

That works for me. I just rebased my fork's commits onto upstream/master and squashed into a single commit.
I'll do a final read through the code this morning and mark this ready for review.

Erik1000 · 2026-03-17T11:51:03Z

@ddimaria This PR is already very large. It would be best to get this stabilized first (is it suitable for review now?).
You can create other PRs that are dependent on this on. Conditional formatting and Data Validation should be separate PRs.

That works for me. I just rebased my fork's commits onto upstream/master and squashed into a single commit. I'll do a final read through the code this morning and mark this ready for review.

is this PR now ready for review and can be merge? 👀

ddimaria · 2026-03-17T13:01:11Z

@ddimaria This PR is already very large. It would be best to get this stabilized first (is it suitable for review now?).
You can create other PRs that are dependent on this on. Conditional formatting and Data Validation should be separate PRs.

That works for me. I just rebased my fork's commits onto upstream/master and squashed into a single commit. I'll do a final read through the code this morning and mark this ready for review.

is this PR now ready for review and can be merge? 👀

@Erik1000 I believe it is, though some conflicts just popped up. Let me review those and CI and it will be ready to review/merge.

…worksheet layout Made-with: Cursor

ddimaria · 2026-03-19T16:59:15Z

@jmcnamara this PR is ready for review. I'm staring conditional formatting in another branch.

jmcnamara · 2026-03-19T17:39:16Z

this PR is ready for review. I'm staring conditional formatting in another branch.

Ok. Fix the cargo fmt issues and if present, any clippy warnings. I think it would be best to resubmit this as a new PR. There are a lot of out of date comments on this PR.

ddimaria mentioned this pull request Jul 24, 2025

Extract more information at the cell level #513

Open