Use a BTreeSet for struct fields to assure stable hashing (#458)

partiql · Apr 4, 2024 · 00c21e2 · 00c21e2 · github-actions · Apr 4, 2024
1 parent 17886f3
commit 00c21e2
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 64 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 
 ### Fixed
+- partiql-types: Fixed handling of struct fields to be resilient to field order w.r.t. equality and hashing 
 
 ## [0.7.1] - 2024-03-15
 ### Changed

diff --git a/Cargo.toml b/Cargo.toml
@@ -2,34 +2,33 @@
 authors = ["PartiQL Team <[email protected]>"]
 homepage = "https://github.com/partiql/partiql-lang-rust"
 repository = "https://github.com/partiql/partiql-lang-rust"
-version = "0.7.1"
+version = "0.7.2"
 edition = "2021"
 
 [workspace]
 resolver = "2"
 
 members = [
-  "partiql",
-  "partiql-ast",
-  "partiql-ast/partiql-ast-macros",
-  "partiql-ast-passes",
-  "partiql-catalog",
-  "partiql-conformance-tests",
-  "partiql-conformance-test-generator",
-  "partiql-source-map",
-  "partiql-logical-planner",
-  "partiql-logical",
-  "partiql-eval",
-  "partiql-ir",
-  "partiql-irgen",
-  "partiql-parser",
-  "partiql-rewriter",
-  "partiql-types",
-  "partiql-value",
-
-  "extension/partiql-extension-ion",
-  "extension/partiql-extension-ion-functions",
-  "extension/partiql-extension-visualize",
+    "partiql",
+    "partiql-ast",
+    "partiql-ast/partiql-ast-macros",
+    "partiql-ast-passes",
+    "partiql-catalog",
+    "partiql-conformance-tests",
+    "partiql-conformance-test-generator",
+    "partiql-source-map",
+    "partiql-logical-planner",
+    "partiql-logical",
+    "partiql-eval",
+    "partiql-ir",
+    "partiql-irgen",
+    "partiql-parser",
+    "partiql-rewriter",
+    "partiql-types",
+    "partiql-value",
+    "extension/partiql-extension-ion",
+    "extension/partiql-extension-ion-functions",
+    "extension/partiql-extension-visualize",
 ]
 
 [profile.dev.build-override]

diff --git a/partiql-logical-planner/src/typer.rs b/partiql-logical-planner/src/typer.rs
@@ -202,18 +202,13 @@ impl<'c> PlanTyper<'c> {
                 }
             }
             BindingsOp::Project(partiql_logical::Project { exprs }) => {
-                let mut fields = vec![];
-                for (k, v) in exprs {
+                let fields = exprs.iter().map(|(k, v)| {
                     self.type_vexpr(v, LookupOrder::LocalGlobal);
-
-                    fields.push(StructField::new(
-                        k.as_str(),
-                        self.get_singleton_type_from_env(),
-                    ));
-                }
+                    StructField::new(k.as_str(), self.get_singleton_type_from_env())
+                });
 
                 let ty = PartiqlType::new_struct(StructType::new(BTreeSet::from([
-                    StructConstraint::Fields(fields),
+                    StructConstraint::Fields(fields.collect()),
                 ])));
 
                 let derived_type_ctx = self.local_type_ctx();
@@ -598,11 +593,12 @@ mod tests {
             "SELECT customers.id, customers.name FROM customers",
             create_customer_schema(
                 false,
-                vec![
+                [
                     StructField::new("id", int!()),
                     StructField::new("name", str!()),
                     StructField::new("age", any!()),
-                ],
+                ]
+                .into(),
             ),
             vec![
                 StructField::new("id", int!()),
@@ -617,11 +613,12 @@ mod tests {
             "SELECT id, customers.name FROM customers",
             create_customer_schema(
                 false,
-                vec![
+                [
                     StructField::new("id", int!()),
                     StructField::new("name", str!()),
                     StructField::new("age", any!()),
-                ],
+                ]
+                .into(),
             ),
             vec![
                 StructField::new("id", int!()),
@@ -636,11 +633,12 @@ mod tests {
             "SELECT customers.id, customers.name, customers.age FROM customers",
             create_customer_schema(
                 true,
-                vec![
+                [
                     StructField::new("id", int!()),
                     StructField::new("name", str!()),
                     StructField::new("age", any!()),
-                ],
+                ]
+                .into(),
             ),
             vec![
                 StructField::new("id", int!()),
@@ -655,10 +653,11 @@ mod tests {
             "SELECT customers.id, customers.name, customers.age FROM customers",
             create_customer_schema(
                 false,
-                vec![
+                [
                     StructField::new("id", int!()),
                     StructField::new("name", str!()),
-                ],
+                ]
+                .into(),
             ),
             vec![
                 StructField::new("id", int!()),
@@ -677,11 +676,12 @@ mod tests {
             "SELECT customers.id, customers.name, customers.details.age FROM customers",
             create_customer_schema(
                 true,
-                vec![
+                [
                     StructField::new("id", int!()),
                     StructField::new("name", str!()),
                     StructField::new("details", details.clone()),
-                ],
+                ]
+                .into(),
             ),
             vec![
                 StructField::new("id", int!()),
@@ -695,19 +695,19 @@ mod tests {
         assert_query_typing(
             TypingMode::Strict,
             "SELECT customers.id, customers.name, customers.details.age, customers.details.foo.bar FROM customers",
-            create_customer_schema(true,vec![
+            create_customer_schema(true, [
                 StructField::new("id", int!()),
                 StructField::new("name", str!()),
                 StructField::new("details", details.clone()),
-            ]),
+            ].into()),
             vec![
                 StructField::new("id", int!()),
                 StructField::new("name", str!()),
                 StructField::new("age", int!()),
                 StructField::new("bar", any!()),
             ],
         )
-        .expect("Type");
+            .expect("Type");
     }
 
     #[test]
@@ -723,11 +723,12 @@ mod tests {
             "SELECT d.age FROM customers.details AS d",
             create_customer_schema(
                 true,
-                vec![
+                [
                     StructField::new("id", int!()),
                     StructField::new("name", str!()),
                     StructField::new("details", details.clone()),
-                ],
+                ]
+                .into(),
             ),
             vec![StructField::new("age", int!())],
         )
@@ -739,11 +740,12 @@ mod tests {
             "SELECT c.id AS my_id, customers.name AS my_name FROM customers AS c",
             create_customer_schema(
                 false,
-                vec![
+                [
                     StructField::new("id", int!()),
                     StructField::new("name", str!()),
                     StructField::new("age", any!()),
-                ],
+                ]
+                .into(),
             ),
             vec![
                 StructField::new("my_id", int!()),
@@ -756,18 +758,19 @@ mod tests {
     #[test]
     fn simple_sfw_err() {
         // Closed Schema with `Strict` typing mode and `age` non-existent projection.
-        let err1 = r#"No Typing Information for SymbolPrimitive { value: "age", case: CaseInsensitive } in closed Schema PartiqlType(Struct(StructType { constraints: {Open(false), Fields([StructField { name: "id", ty: PartiqlType(Int) }, StructField { name: "name", ty: PartiqlType(String) }])} }))"#;
+        let err1 = r#"No Typing Information for SymbolPrimitive { value: "age", case: CaseInsensitive } in closed Schema PartiqlType(Struct(StructType { constraints: {Open(false), Fields({StructField { name: "id", ty: PartiqlType(Int) }, StructField { name: "name", ty: PartiqlType(String) }})} }))"#;
 
         assert_err(
             assert_query_typing(
                 TypingMode::Strict,
                 "SELECT customers.id, customers.name, customers.age FROM customers",
                 create_customer_schema(
                     false,
-                    vec![
+                    [
                         StructField::new("id", int!()),
                         StructField::new("name", str!()),
-                    ],
+                    ]
+                    .into(),
                 ),
                 vec![],
             ),
@@ -776,11 +779,12 @@ mod tests {
                 // TypingError::IllegalState(err2.to_string()),
             ],
             Some(bag![r#struct![BTreeSet::from([StructConstraint::Fields(
-                vec![
+                [
                     StructField::new("id", int!()),
                     StructField::new("name", str!()),
                     StructField::new("age", undefined!()),
                 ]
+                .into()
             ),])]]),
         );
 
@@ -791,7 +795,7 @@ mod tests {
             StructConstraint::Open(false)
         ])];
 
-        let err1 = r#"No Typing Information for SymbolPrimitive { value: "details", case: CaseInsensitive } in closed Schema PartiqlType(Struct(StructType { constraints: {Open(false), Fields([StructField { name: "age", ty: PartiqlType(Int) }])} }))"#;
+        let err1 = r#"No Typing Information for SymbolPrimitive { value: "details", case: CaseInsensitive } in closed Schema PartiqlType(Struct(StructType { constraints: {Open(false), Fields({StructField { name: "age", ty: PartiqlType(Int) }})} }))"#;
         let err2 = r"Illegal Derive Type PartiqlType(Undefined)";
 
         assert_err(
@@ -800,11 +804,12 @@ mod tests {
                 "SELECT customers.id, customers.name, customers.details.bar FROM customers",
                 create_customer_schema(
                     false,
-                    vec![
+                    [
                         StructField::new("id", int!()),
                         StructField::new("name", str!()),
                         StructField::new("details", details),
-                    ],
+                    ]
+                    .into(),
                 ),
                 vec![],
             ),
@@ -813,11 +818,12 @@ mod tests {
                 TypingError::IllegalState(err2.to_string()),
             ],
             Some(bag![r#struct![BTreeSet::from([StructConstraint::Fields(
-                vec![
+                [
                     StructField::new("id", int!()),
                     StructField::new("name", str!()),
                     StructField::new("bar", undefined!()),
                 ]
+                .into()
             ),])]]),
         );
     }
@@ -836,14 +842,14 @@ mod tests {
                     e,
                     TypeErr {
                         errors: expected_errors,
-                        output
+                        output,
                     }
                 );
             }
         };
     }
 
-    fn create_customer_schema(is_open: bool, fields: Vec<StructField>) -> PartiqlType {
+    fn create_customer_schema(is_open: bool, fields: BTreeSet<StructField>) -> PartiqlType {
         bag![r#struct![BTreeSet::from([
             StructConstraint::Fields(fields),
             StructConstraint::Open(is_open)
@@ -856,6 +862,7 @@ mod tests {
         schema: PartiqlType,
         expected_fields: Vec<StructField>,
     ) -> Result<(), TypeErr> {
+        let expected_fields: BTreeSet<_> = expected_fields.into_iter().collect();
         let actual = type_query(mode, query, TypeEnvEntry::new("customers", &[], schema));
 
         match actual {

diff --git a/partiql-types/src/lib.rs b/partiql-types/src/lib.rs
@@ -109,7 +109,7 @@ macro_rules! r#struct {
 #[macro_export]
 macro_rules! struct_fields {
     ($(($x:expr, $y:expr)),+ $(,)?) => (
-        $crate::StructConstraint::Fields(vec![$(($x, $y).into()),+])
+        $crate::StructConstraint::Fields([$(($x, $y).into()),+].into())
     );
 }
 
@@ -397,14 +397,14 @@ impl StructType {
     }
 
     #[must_use]
-    pub fn fields(&self) -> Vec<StructField> {
+    pub fn fields(&self) -> BTreeSet<StructField> {
         self.constraints
             .iter()
             .flat_map(|c| {
                 if let StructConstraint::Fields(fields) = c.clone() {
                     fields
                 } else {
-                    vec![]
+                    Default::default()
                 }
             })
             .collect()
@@ -428,7 +428,7 @@ pub enum StructConstraint {
     Open(bool),
     Ordered(bool),
     DuplicateAttrs(bool),
-    Fields(Vec<StructField>),
+    Fields(BTreeSet<StructField>),
 }
 
 #[derive(Debug, Clone, Eq, PartialEq, Hash, Ord, PartialOrd)]

diff --git a/partiql/src/subquery_tests.rs b/partiql/src/subquery_tests.rs
@@ -89,7 +89,7 @@ mod tests {
         let res = evaluate(&catalog, plan, bindings, &[]).expect("should eval correctly");
         dbg!(&res);
         assert!(res != Value::Missing);
-        assert_eq!(res, Value::from(bag![tuple![("a", "b")]]))
+        assert_eq!(res, Value::from(bag![tuple![("a", "b")]]));
     }
 
     #[test]
@@ -150,6 +150,6 @@ mod tests {
         let res = evaluate(&catalog, plan, bindings, &[]).expect("should eval correctly");
         dbg!(&res);
         assert!(res != Value::Missing);
-        assert_eq!(res, Value::from(bag![tuple![("a", "b")]]))
+        assert_eq!(res, Value::from(bag![tuple![("a", "b")]]));
     }
 }
Benchmark suite	Current: `00c21e2`	Previous: `17886f3`	Ratio
`arith_agg-avg`	`747530` ns/iter (`± 7627`)	`753690` ns/iter (`± 4102`)	`0.99`
`arith_agg-avg_distinct`	`826076` ns/iter (`± 3422`)	`844147` ns/iter (`± 2396`)	`0.98`
`arith_agg-count`	`802113` ns/iter (`± 15848`)	`795454` ns/iter (`± 13612`)	`1.01`
`arith_agg-count_distinct`	`822988` ns/iter (`± 34472`)	`837464` ns/iter (`± 6981`)	`0.98`
`arith_agg-min`	`814104` ns/iter (`± 15520`)	`803174` ns/iter (`± 24688`)	`1.01`
`arith_agg-min_distinct`	`826390` ns/iter (`± 3202`)	`837571` ns/iter (`± 4577`)	`0.99`
`arith_agg-max`	`814851` ns/iter (`± 3125`)	`807597` ns/iter (`± 15131`)	`1.01`
`arith_agg-max_distinct`	`837009` ns/iter (`± 6319`)	`847482` ns/iter (`± 2965`)	`0.99`
`arith_agg-sum`	`798883` ns/iter (`± 3890`)	`796797` ns/iter (`± 2873`)	`1.00`
`arith_agg-sum_distinct`	`826630` ns/iter (`± 5629`)	`839367` ns/iter (`± 1810`)	`0.98`
`arith_agg-avg-count-min-max-sum`	`953800` ns/iter (`± 4348`)	`938359` ns/iter (`± 3028`)	`1.02`
`arith_agg-avg-count-min-max-sum-group_by`	`1186162` ns/iter (`± 40553`)	`1189135` ns/iter (`± 18550`)	`1.00`
`arith_agg-avg-count-min-max-sum-group_by-group_as`	`1750674` ns/iter (`± 38977`)	`1740268` ns/iter (`± 11739`)	`1.01`
`arith_agg-avg_distinct-count_distinct-min_distinct-max_distinct-sum_distinct`	`1177287` ns/iter (`± 8132`)	`1225613` ns/iter (`± 20330`)	`0.96`
`arith_agg-avg_distinct-count_distinct-min_distinct-max_distinct-sum_distinct-group_by`	`1437098` ns/iter (`± 43249`)	`1484296` ns/iter (`± 71180`)	`0.97`
`arith_agg-avg_distinct-count_distinct-min_distinct-max_distinct-sum_distinct-group_by-group_as`	`1991738` ns/iter (`± 10800`)	`2063772` ns/iter (`± 49932`)	`0.97`
`parse-1`	`4168` ns/iter (`± 149`)	`4208` ns/iter (`± 85`)	`0.99`
`parse-15`	`39006` ns/iter (`± 604`)	`38499` ns/iter (`± 179`)	`1.01`
`parse-30`	`79630` ns/iter (`± 805`)	`75779` ns/iter (`± 262`)	`1.05`
`compile-1`	`4455` ns/iter (`± 73`)	`4322` ns/iter (`± 27`)	`1.03`
`compile-15`	`32603` ns/iter (`± 225`)	`30864` ns/iter (`± 108`)	`1.06`
`compile-30`	`66751` ns/iter (`± 301`)	`63361` ns/iter (`± 115`)	`1.05`
`plan-1`	`66767` ns/iter (`± 546`)	`68883` ns/iter (`± 450`)	`0.97`
`plan-15`	`1043840` ns/iter (`± 9008`)	`1067893` ns/iter (`± 50804`)	`0.98`
`plan-30`	`2095665` ns/iter (`± 11711`)	`2153620` ns/iter (`± 7001`)	`0.97`
`eval-1`	`12852768` ns/iter (`± 192002`)	`12510282` ns/iter (`± 51621`)	`1.03`
`eval-15`	`86187523` ns/iter (`± 765829`)	`85224979` ns/iter (`± 936441`)	`1.01`
`eval-30`	`165518632` ns/iter (`± 988872`)	`163846964` ns/iter (`± 403213`)	`1.01`
`join`	`9710` ns/iter (`± 76`)	`9807` ns/iter (`± 49`)	`0.99`
`simple`	`2420` ns/iter (`± 6`)	`2517` ns/iter (`± 13`)	`0.96`
`simple-no`	`430` ns/iter (`± 2`)	`476` ns/iter (`± 5`)	`0.90`
`numbers`	`48` ns/iter (`± 0`)	`58` ns/iter (`± 0`)	`0.83`
`parse-simple`	`566` ns/iter (`± 2`)	`553` ns/iter (`± 11`)	`1.02`
`parse-ion`	`1731` ns/iter (`± 7`)	`1754` ns/iter (`± 6`)	`0.99`
`parse-group`	`5627` ns/iter (`± 9`)	`5894` ns/iter (`± 55`)	`0.95`
`parse-complex`	`14913` ns/iter (`± 51`)	`14875` ns/iter (`± 50`)	`1.00`
`parse-complex-fexpr`	`21258` ns/iter (`± 168`)	`21550` ns/iter (`± 210`)	`0.99`