From 00c21e2759cbf2fdf266ec9ff58d014bdcf4abaf Mon Sep 17 00:00:00 2001 From: Josh Pschorr Date: Thu, 4 Apr 2024 11:19:58 -0700 Subject: [PATCH] Use a BTreeSet for struct fields to assure stable hashing (#458) --- CHANGELOG.md | 1 + Cargo.toml | 43 ++++++++------- partiql-logical-planner/src/typer.rs | 79 +++++++++++++++------------- partiql-types/src/lib.rs | 8 +-- partiql/src/subquery_tests.rs | 4 +- 5 files changed, 71 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66f299cd..9fbb7985 100644 --- 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 index 0d47dc5b..1e09ee85 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,34 +2,33 @@ authors = ["PartiQL Team "] 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 index 3f9fbac4..b1617d57 100644 --- 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,11 +695,11 @@ 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!()), @@ -707,7 +707,7 @@ mod tests { 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,7 +758,7 @@ 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( @@ -764,10 +766,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![], ), @@ -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) -> PartiqlType { + fn create_customer_schema(is_open: bool, fields: BTreeSet) -> PartiqlType { bag![r#struct![BTreeSet::from([ StructConstraint::Fields(fields), StructConstraint::Open(is_open) @@ -856,6 +862,7 @@ mod tests { schema: PartiqlType, expected_fields: Vec, ) -> 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 index 47294f16..701c1f1e 100644 --- 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 { + pub fn fields(&self) -> BTreeSet { 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), + Fields(BTreeSet), } #[derive(Debug, Clone, Eq, PartialEq, Hash, Ord, PartialOrd)] diff --git a/partiql/src/subquery_tests.rs b/partiql/src/subquery_tests.rs index d8a7ab2c..9006f344 100644 --- 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")]])); } }