validate paths before we insert

Signed-off-by: Andrew Duffy <[email protected]>

Author: a10y

vortex-dtype/src/field.rs

@@ -206,6 +206,12 @@ impl FieldPath {
         // Indexing a struct type always allocates anyway.
         self.resolve(dtype).is_some()
     }
+
+    /// Returns true if this path overlaps with another path (i.e., one is a prefix of the other).
+    pub fn overlap(&self, other: &FieldPath) -> bool {
+        let min_len = self.0.len().min(other.0.len());
+        self.0.iter().take(min_len).eq(other.0.iter().take(min_len))
+    }
 }
 
 impl FromIterator<Field> for FieldPath {
@@ -388,4 +394,28 @@ mod tests {
         assert!(path.resolve(dtype.clone()).is_none());
         assert!(!path.exists_in(dtype));
     }
+
+    #[test]
+    fn test_overlap_positive() {
+        let path1 = field_path!(a.b.c);
+        let path2 = field_path!(a.b);
+        assert!(path1.overlap(&path2));
+        assert!(path2.overlap(&path1));
+
+        let path3 = field_path!(a);
+        assert!(path1.overlap(&path3));
+        assert!(path3.overlap(&path1));
+    }
+
+    #[test]
+    fn test_overlap_negative() {
+        let path1 = field_path!(a.b.c);
+        let path2 = field_path!(a.x.y);
+        assert!(!path1.overlap(&path2));
+        assert!(!path2.overlap(&path1));
+
+        let path3 = field_path!(x);
+        assert!(!path1.overlap(&path3));
+        assert!(!path3.overlap(&path1));
+    }
 }

vortex-layout/src/layouts/path.rs

@@ -33,6 +33,7 @@ use vortex_utils::aliases::hash_set::HashSet;
 use crate::IntoLayout;
 use crate::LayoutRef;
 use crate::LayoutStrategy;
+use crate::layouts::flat::writer::FlatLayoutStrategy;
 use crate::layouts::struct_::StructLayout;
 use crate::segments::SegmentSinkRef;
 use crate::sequence::SendableSequentialStream;
@@ -52,6 +53,18 @@ pub struct PathStrategy {
     fallback: Arc<dyn LayoutStrategy>,
 }
 
+impl Default for PathStrategy {
+    fn default() -> Self {
+        let flat = Arc::new(FlatLayoutStrategy::default());
+
+        Self {
+            leaf_writers: HashMap::default(),
+            validity: flat.clone(),
+            fallback: flat,
+        }
+    }
+}
+
 impl PathStrategy {
     /// Create a new writer with the specified write strategies for validity, and for all leaf
     /// fields, with no overrides.
@@ -113,7 +126,8 @@ impl PathStrategy {
         field_path: impl Into<FieldPath>,
         writer: Arc<dyn LayoutStrategy>,
     ) -> Self {
-        self.leaf_writers.insert(field_path.into(), writer);
+        self.leaf_writers
+            .insert(self.validate_path(field_path.into()), writer);
         self
     }
 
@@ -124,12 +138,16 @@ impl PathStrategy {
         mut self,
         writers: impl IntoIterator<Item = (FieldPath, Arc<dyn LayoutStrategy>)>,
     ) -> Self {
-        self.leaf_writers.extend(writers);
+        for (field_path, strategy) in writers {
+            self.leaf_writers
+                .insert(self.validate_path(field_path), strategy);
+        }
         self
     }
 }
 
 impl PathStrategy {
+    // Descend into a subfield for the writer.
     fn descend(&self, field: &Field) -> Self {
         // Start with the existing set of overrides, then only retain the ones that contain
         // the current field
@@ -149,6 +167,19 @@ impl PathStrategy {
             fallback: self.fallback.clone(),
         }
     }
+
+    fn validate_path(&self, path: FieldPath) -> FieldPath {
+        // Validate that the field path does not conflict with any overrides
+        // that we've added by overlapping.
+        for field_path in self.leaf_writers.keys() {
+            assert!(
+                !path.overlap(field_path),
+                "Override for field_path {path} conflicts with existing override for {field_path}"
+            );
+        }
+
+        path
+    }
 }
 
 /// Specialized strategy for when we exactly know the input schema.
@@ -322,3 +353,27 @@ impl LayoutStrategy for PathStrategy {
         Ok(StructLayout::new(row_count, dtype, column_layouts).into_layout())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::sync::Arc;
+
+    use vortex_dtype::field_path;
+
+    use crate::layouts::flat::writer::FlatLayoutStrategy;
+    use crate::layouts::path::PathStrategy;
+
+    #[test]
+    #[should_panic(
+        expected = "Override for field_path $a.$b conflicts with existing override for $a.$b.$c"
+    )]
+    fn test_overlapping_paths_fail() {
+        let flat = Arc::new(FlatLayoutStrategy::default());
+
+        // Success
+        let path = PathStrategy::default().with_field_writer(field_path!(a.b.c), flat.clone());
+
+        // Should panic right here.
+        let _path = path.with_field_writer(field_path!(a.b), flat);
+    }
+}

vortex-layout/src/layouts/struct_/reader.rs

@@ -417,7 +417,7 @@ mod tests {
     use crate::LayoutRef;
     use crate::LayoutStrategy;
     use crate::layouts::flat::writer::FlatLayoutStrategy;
-    use crate::layouts::struct_::writer::StructStrategy;
+    use crate::layouts::path::PathStrategy;
     use crate::segments::SegmentSource;
     use crate::segments::TestSegments;
     use crate::sequence::SequenceId;
@@ -430,8 +430,10 @@ mod tests {
 
         let segments = Arc::new(TestSegments::default());
         let (ptr, eof) = SequenceId::root().split();
-        let strategy =
-            StructStrategy::new(FlatLayoutStrategy::default(), FlatLayoutStrategy::default());
+        let strategy = PathStrategy::new(
+            Arc::new(FlatLayoutStrategy::default()),
+            Arc::new(FlatLayoutStrategy::default()),
+        );
         let layout = block_on(|handle| {
             strategy.write_stream(
                 ctx,
@@ -461,8 +463,10 @@ mod tests {
         let ctx = ArrayContext::empty();
         let segments = Arc::new(TestSegments::default());
         let (ptr, eof) = SequenceId::root().split();
-        let strategy =
-            StructStrategy::new(FlatLayoutStrategy::default(), FlatLayoutStrategy::default());
+        let strategy = PathStrategy::new(
+            Arc::new(FlatLayoutStrategy::default()),
+            Arc::new(FlatLayoutStrategy::default()),
+        );
         let layout = block_on(|handle| {
             strategy.write_stream(
                 ctx,
@@ -495,8 +499,10 @@ mod tests {
 
         let segments = Arc::new(TestSegments::default());
         let (ptr, eof) = SequenceId::root().split();
-        let strategy =
-            StructStrategy::new(FlatLayoutStrategy::default(), FlatLayoutStrategy::default());
+        let strategy = PathStrategy::new(
+            Arc::new(FlatLayoutStrategy::default()),
+            Arc::new(FlatLayoutStrategy::default()),
+        );
         let layout = block_on(|handle| {
             strategy.write_stream(
                 ctx,
@@ -534,8 +540,10 @@ mod tests {
         let ctx = ArrayContext::empty();
         let segments = Arc::new(TestSegments::default());
         let (ptr, eof) = SequenceId::root().split();
-        let strategy =
-            StructStrategy::new(FlatLayoutStrategy::default(), FlatLayoutStrategy::default());
+        let strategy = PathStrategy::new(
+            Arc::new(FlatLayoutStrategy::default()),
+            Arc::new(FlatLayoutStrategy::default()),
+        );
         let layout = block_on(|handle| {
             strategy.write_stream(
                 ctx,

vortex-layout/src/layouts/struct_/writer.rs

@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: Apache-2.0
 // SPDX-FileCopyrightText: Copyright the Vortex contributors
 
+#![allow(deprecated, reason = "This module is deprecated")]
+
 use std::sync::Arc;
 
 use async_trait::async_trait;