more fixes
authorBen Pfaff <blp@cs.stanford.edu>
Wed, 7 Jan 2026 17:08:45 +0000 (09:08 -0800)
committerBen Pfaff <blp@cs.stanford.edu>
Wed, 7 Jan 2026 17:08:45 +0000 (09:08 -0800)
rust/pspp/src/output/pivot.rs
rust/pspp/src/output/pivot/output.rs
rust/pspp/src/spv/read/light.rs
rust/pspp/src/spv/read/tests.rs
rust/pspp/src/spv/testdata/light2.expected

index d37fc0732f098e8894072b03cb51a06ad4129a3c..6a033c451d3d7dbdc129e6272bf8fa385b63a1f2 100644 (file)
@@ -168,18 +168,12 @@ impl PivotTable {
     /// [with_title]: Self::with_title
     /// [with_data]: Self::with_data
     /// [insert]: Self::insert
-    pub fn new(dimensions: impl IntoIterator<Item = (Axis3, Dimension)>) -> Self {
-        let mut dims = Vec::new();
-        let mut axes = EnumMap::<Axis3, Axis>::default();
-        for (axis, dimension) in dimensions {
-            axes[axis].dimensions.push(dims.len());
-            dims.push(dimension);
-        }
+    pub fn new(structure: impl Into<Structure>) -> Self {
+        let structure = structure.into();
         Self {
             style: PivotTableStyle::default().with_look(Settings::global().look.clone()),
-            layer: repeat_n(0, axes[Axis3::Z].dimensions.len()).collect(),
-            axes,
-            dimensions: dims,
+            layer: repeat_n(0, structure.axes[Axis3::Z].dimensions.len()).collect(),
+            structure,
             ..Self::default()
         }
     }
@@ -377,7 +371,7 @@ impl PivotTable {
     /// Returns an iterator for all the values along `axis`.
     fn axis_values(&self, axis: Axis3) -> AxisIterator {
         AxisIterator {
-            indexes: repeat_n(0, self.axes[axis].dimensions.len()).collect(),
+            indexes: repeat_n(0, self.structure.axes[axis].dimensions.len()).collect(),
             lengths: self.axis_dimensions(axis).map(|d| d.len()).collect(),
             done: self.axis_extent(axis) == 0,
         }
@@ -394,12 +388,12 @@ impl PivotTable {
 
     /// Returns the pivot table's dimensions.
     pub fn dimensions(&self) -> &[Dimension] {
-        &self.dimensions
+        &self.structure.dimensions
     }
 
     /// Returns the pivot table's axes.
     pub fn axes(&self) -> &EnumMap<Axis3, Axis> {
-        &self.axes
+        &self.structure.axes
     }
 
     /// Returns the pivot table's [Look], for modification.
@@ -443,7 +437,7 @@ impl PivotTable {
     where
         C: CellIndex,
     {
-        cell_index.cell_index(self.dimensions.iter().map(|d| d.len()))
+        cell_index.cell_index(self.dimensions().iter().map(|d| d.len()))
     }
 
     /// Inserts a cell with the given `value` and `cell_index`.
@@ -478,14 +472,14 @@ impl PivotTable {
         &self,
         presentation_indexes: EnumMap<Axis3, &[usize]>,
     ) -> SmallVec<[usize; 4]> {
-        let mut data_indexes = SmallVec::from_elem(0, self.dimensions.len());
+        let mut data_indexes = SmallVec::from_elem(0, self.dimensions().len());
         for (axis, presentation_indexes) in presentation_indexes {
-            for (&dim_index, &pindex) in self.axes[axis]
+            for (&dim_index, &pindex) in self.structure.axes[axis]
                 .dimensions
                 .iter()
                 .zip(presentation_indexes.iter())
             {
-                data_indexes[dim_index] = self.dimensions[dim_index].ptod[pindex];
+                data_indexes[dim_index] = self.structure.dimensions[dim_index].ptod[pindex];
             }
         }
         data_indexes
@@ -507,7 +501,7 @@ impl PivotTable {
 
     /// Transposes row and columns.
     pub fn transpose(&mut self) {
-        self.axes.swap(Axis3::X, Axis3::Y);
+        self.structure.axes.swap(Axis3::X, Axis3::Y);
     }
 
     /// Returns an iterator through dimensions on the given `axis`.
@@ -515,17 +509,17 @@ impl PivotTable {
         &self,
         axis: Axis3,
     ) -> impl DoubleEndedIterator<Item = &Dimension> + ExactSizeIterator {
-        self.axes[axis]
+        self.structure.axes[axis]
             .dimensions
             .iter()
             .copied()
-            .map(|index| &self.dimensions[index])
+            .map(|index| &self.structure.dimensions[index])
     }
 
     fn find_dimension(&self, dim_index: usize) -> Option<(Axis3, usize)> {
-        debug_assert!(dim_index < self.dimensions.len());
+        debug_assert!(dim_index < self.structure.dimensions.len());
         for axis in enum_iterator::all::<Axis3>() {
-            for (position, dimension) in self.axes[axis].dimensions.iter().copied().enumerate() {
+            for (position, dimension) in self.axes()[axis].dimensions.iter().copied().enumerate() {
                 if dimension == dim_index {
                     return Some((axis, position));
                 }
@@ -575,8 +569,10 @@ impl PivotTable {
             _ => (),
         }
 
-        self.axes[old_axis].dimensions.remove(old_position);
-        self.axes[new_axis]
+        self.structure.axes[old_axis]
+            .dimensions
+            .remove(old_position);
+        self.structure.axes[new_axis]
             .dimensions
             .insert(new_position, dim_index);
     }
@@ -1724,11 +1720,9 @@ pub struct PivotTable {
     /// Footnotes.
     pub footnotes: Footnotes,
 
-    /// Dimensions.
-    dimensions: Vec<Dimension>,
-
-    /// Axes.
-    axes: EnumMap<Axis3, Axis>,
+    /// Table structure.
+    #[serde(flatten)]
+    structure: Structure,
 
     /// Data.
     cells: HashMap<usize, Value>,
@@ -1741,13 +1735,55 @@ impl Default for PivotTable {
             metadata: PivotTableMetadata::default(),
             layer: Vec::new(),
             footnotes: Footnotes::new(),
-            dimensions: Vec::new(),
-            axes: EnumMap::default(),
+            structure: Structure::default(),
             cells: HashMap::new(),
         }
     }
 }
 
+/// The structure of a pivot table.
+#[derive(Clone, Debug, Default, Serialize)]
+pub struct Structure {
+    /// Dimensions.
+    dimensions: Vec<Dimension>,
+
+    /// Axes.
+    axes: EnumMap<Axis3, Axis>,
+}
+
+/// [Structure::new] error for a missing or duplicate dimension among the axes.
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
+pub struct InvalidAxes;
+
+impl Structure {
+    /// Creates a new `Structure` from the arguments.
+    ///
+    /// Usually [Structure::from] is easier.
+    pub fn new(
+        dimensions: Vec<Dimension>,
+        axes: EnumMap<Axis3, Axis>,
+    ) -> Result<Self, InvalidAxes> {
+        // XXX validate axes
+        Ok(Self { dimensions, axes })
+    }
+}
+
+impl<T> From<T> for Structure
+where
+    T: IntoIterator<Item = (Axis3, Dimension)>,
+{
+    fn from(value: T) -> Self {
+        let mut structure = Structure::default();
+        for (axis, dimension) in value {
+            structure.axes[axis]
+                .dimensions
+                .push(structure.dimensions.len());
+            structure.dimensions.push(dimension);
+        }
+        structure
+    }
+}
+
 /// A type that can calculate the index into a [PivotTable]'s data hashmap.
 pub trait CellIndex {
     /// Given the pivot table's `dimensions`, returns an index.
index b4d1db288520221b9bd1c18ab44037517106cd2b..6c5040295b28328add016d4c2bc20bdb79370084 100644 (file)
@@ -108,7 +108,7 @@ impl PivotTable {
         layer_indexes: &[usize],
         omit_empty: bool,
     ) -> AxisEnumeration {
-        let axis = &self.axes[enum_axis];
+        let axis = &self.structure.axes[enum_axis];
         let extent = self.axis_extent(enum_axis);
         let indexes = if axis.dimensions.is_empty() {
             vec![0]
@@ -294,10 +294,10 @@ impl PivotTable {
     /// Constructs a [Table] for this `PivotTable`'s layer values.  Returns
     /// `None` if the table doesn't have layers.
     pub fn output_layers(&self, layer_indexes: &[usize]) -> Vec<Table> {
-        self.axes[Axis3::Z]
+        self.structure.axes[Axis3::Z]
             .dimensions
             .iter()
-            .map(|index| &self.dimensions[*index])
+            .map(|index| &self.structure.dimensions[*index])
             .zip(layer_indexes)
             .rev()
             .filter(|(dimension, _)| !dimension.is_empty())
@@ -379,11 +379,11 @@ impl PivotTable {
     }
 
     fn nonempty_layer_dimensions(&self) -> impl Iterator<Item = &Dimension> {
-        self.axes[Axis3::Z]
+        self.structure.axes[Axis3::Z]
             .dimensions
             .iter()
             .rev()
-            .map(|index| &self.dimensions[*index])
+            .map(|index| &self.structure.dimensions[*index])
             .filter(|d| !d.root.is_empty())
     }
 
@@ -653,14 +653,18 @@ impl<'a> Headings<'a> {
         let column_enumeration =
             pt.enumerate_axis(h.into(), layer_indexes, pt.style.look.hide_empty);
 
-        let mut headings = pt.axes[h.into()]
+        let mut headings = pt.structure.axes[h.into()]
             .dimensions
             .iter()
             .copied()
             .enumerate()
             .rev()
             .filter_map(|(axis_index, dim_index)| {
-                Heading::new(&pt.dimensions[dim_index], axis_index, &column_enumeration)
+                Heading::new(
+                    &pt.structure.dimensions[dim_index],
+                    axis_index,
+                    &column_enumeration,
+                )
             })
             .collect::<Vec<_>>();
 
index 3c0851a4fd03598ebe3ffee9ad1cbfcf37b6f837..a3c685432ca466f3815847f18b1fd59f6001fdb0 100644 (file)
@@ -1,7 +1,7 @@
 use std::{
     fmt::Debug,
     io::{Read, Seek},
-    iter::{repeat, zip},
+    iter::repeat,
     ops::Deref,
     str::FromStr,
     sync::Arc,
@@ -23,8 +23,8 @@ use crate::{
         UncheckedFormat, Width,
     },
     output::pivot::{
-        self, Axis2, Axis3, FootnoteMarkerPosition, FootnoteMarkerType, Footnotes, Group,
-        PivotTable, PivotTableMetadata, PivotTableStyle, PrecomputedIndex,
+        self, Axis, Axis2, Axis3, FootnoteMarkerPosition, FootnoteMarkerType, Footnotes, Group,
+        PivotTable, PivotTableMetadata, PivotTableStyle, PrecomputedIndex, Structure,
         look::{
             self, AreaStyle, BoxBorder, Color, HeadingRegion, HorzAlign, LabelPosition, Look,
             RowColBorder, RowParity, Stroke, VertAlign,
@@ -203,14 +203,16 @@ impl LightTable {
                 dimension
             })
             .collect::<Vec<_>>();
-        let axes = self
-            .axes
-            .decode(dimensions.len())
-            .unwrap_or_else(|warning| {
+        let structure = match self.axes.decode(dimensions.len()) {
+            Ok(axes) => {
+                Structure::new(dimensions, axes).expect("Axes::decode pre-validated the structure")
+            }
+            Err(warning) => {
                 warn(warning);
-                vec![Axis3::Y; dimensions.len()]
-            });
-        let pivot_table = PivotTable::new(zip(axes, dimensions))
+                Structure::from(repeat(Axis3::Y).zip(dimensions))
+            }
+        };
+        let pivot_table = PivotTable::new(structure)
             .with_style(PivotTableStyle {
                 look: Arc::new(self.decode_look(encoding)),
                 rotate_inner_column_labels: self.header.rotate_inner_column_labels,
@@ -1753,7 +1755,7 @@ struct Axes {
 }
 
 impl Axes {
-    fn decode(&self, n: usize) -> Result<Vec<Axis3>, LightWarning> {
+    fn decode(&self, n: usize) -> Result<EnumMap<Axis3, Axis>, LightWarning> {
         let actual = self.layers.len() + self.rows.len() + self.columns.len();
         if actual != n {
             return Err(LightWarning::WrongAxisCount {
@@ -1765,24 +1767,25 @@ impl Axes {
             });
         }
 
-        let mut axes = vec![None; n];
-
         fn axis_dims(axis: Axis3, dimensions: &[u32]) -> impl Iterator<Item = (Axis3, usize)> {
             repeat(axis).zip(dimensions.iter().copied().map(|x| x.try_into().unwrap()))
         }
 
+        let mut seen = vec![false; n];
+        let mut axes: EnumMap<Axis3, Axis> = EnumMap::default();
         for (axis, index) in axis_dims(Axis3::Z, &self.layers)
             .chain(axis_dims(Axis3::Y, &self.rows))
             .chain(axis_dims(Axis3::X, &self.columns))
         {
-            match axes.get_mut(index) {
+            match seen.get_mut(index) {
                 None => return Err(LightWarning::InvalidDimensionIndex { index, n }),
-                Some(Some(_)) => return Err(LightWarning::DuplicateDimensionIndex(index)),
-                Some(inner) => *inner = Some(axis),
+                Some(true) => return Err(LightWarning::DuplicateDimensionIndex(index)),
+                Some(other) => *other = true,
             }
+            axes[axis].dimensions.push(index);
         }
 
-        Ok(axes.into_iter().flatten().collect())
+        Ok(axes)
     }
 }
 
index fc38a10dbcbf101b3391fab02abd0fc82a9f776c..ac27ae0d3cd9bf9c6a3842f8c5e752b998de47b2 100644 (file)
@@ -32,6 +32,12 @@ fn light3() {
     test_raw_spvfile("light3", None);
 }
 
+/// Further check for dimensions and axes are ordered properly.
+#[test]
+fn light4() {
+    test_raw_spvfile("light4", None);
+}
+
 #[test]
 fn legacy1() {
     test_raw_spvfile("legacy1", None);
index 5650a27ede02537c0ab57cee8ab1ac2ca49067b9..adb6944f24ae2f7b74e16251c1b41301d890331e 100644 (file)
@@ -4,21 +4,21 @@
 │                              ├──────┬────────┬───────┤      │
 │                              │Normal│Marginal│Extreme│ Total│
 ├──────────────────────────────┼──────┼────────┼───────┼──────┤
-│Variable A Less Count         │    10│     7.0│  45.5%│ 52.6%
-│                Expected Count│ 18.2%│   20.0%│   6.7%│    22
-│                % within A    │ 15.0%│      13│   13.3│ 34.2%│
-│                % within B    │  38.0│  100.0%│  63.3%│ 63.3%│
-│                % of Total    │100.0%│   35.0%│     20│  20.0
+│Variable A Less Count         │    10│       8│      4│    22
+│                Expected Count│   7.0│     7.7│    7.3│  22.0
+│                % within A    │ 45.5%│   36.4%│  18.2%│100.0%│
+│                % within B    │ 52.6%│   38.1%│  20.0%│ 36.7%│
+│                % of Total    │ 16.7%│   13.3%│   6.7%│ 36.7%
 │          ╶───────────────────┼──────┼────────┼───────┼──────┤
-│           More Count         │ 16.7%│       8│    7.7│ 36.4%
-│                Expected Count│  22.0│  100.0%│  36.7%│ 36.7%
-│                % within A    │ 61.9%│   21.7%│     16│  12.7
-│                % within B    │    19│    19.0│  31.7%│100.0%│
-│                % of Total    │ 33.3%│  100.0%│  33.3%│    60
+│           More Count         │     9│      13│     16│    38
+│                Expected Count│  12.0│    13.3│   12.7│  38.0
+│                % within A    │ 23.7%│   34.2%│  42.1%│100.0%
+│                % within B    │ 47.4%│   61.9%│  80.0%│ 63.3%│
+│                % of Total    │ 15.0%│   21.7%│  26.7%│ 63.3%
 ├──────────────────────────────┼──────┼────────┼───────┼──────┤
-│Total           Count         │ 38.1%│   13.3%│      4│   7.3
-│                Expected Count│     9│    12.0│  23.7%│ 47.4%
-│                % within A    │ 42.1%│   80.0%│  26.7%│    38
-│                % within B    │ 31.7%│      21│   21.0│ 35.0%│
-│                % of Total    │  60.0│  100.0%│ 100.0%│100.0%│
+│Total           Count         │    19│      21│     20│    60
+│                Expected Count│  19.0│    21.0│   20.0│  60.0
+│                % within A    │ 31.7%│   35.0%│  33.3%│100.0%
+│                % within B    │100.0%│  100.0%│ 100.0%│100.0%│
+│                % of Total    │ 31.7%│   35.0%│  33.3%│100.0%│
 ╰──────────────────────────────┴──────┴────────┴───────┴──────╯