From 3ed5e2a51ac35f4d15044d61803c80e26a1336a4 Mon Sep 17 00:00:00 2001 From: Jax Liu Date: Wed, 30 Oct 2024 11:47:26 +0800 Subject: [PATCH] feat(core): remove properties field from mdl and improve the error message (#862) --- .../routers/v3/connector/test_postgres.py | 2 +- wren-core-py/src/context.rs | 8 +-- wren-core-py/src/errors.rs | 22 ++++++-- wren-core/core/src/mdl/builder.rs | 51 ------------------- wren-core/core/src/mdl/manifest.rs | 10 ---- 5 files changed, 22 insertions(+), 71 deletions(-) diff --git a/ibis-server/tests/routers/v3/connector/test_postgres.py b/ibis-server/tests/routers/v3/connector/test_postgres.py index cdb29d3f5..34a9afe65 100644 --- a/ibis-server/tests/routers/v3/connector/test_postgres.py +++ b/ibis-server/tests/routers/v3/connector/test_postgres.py @@ -184,7 +184,7 @@ def test_query_with_invalid_manifest_str(postgres: PostgresContainer): }, ) assert response.status_code == 422 - assert response.text == "Invalid padding" + assert response.text == "Base64 decode error: Invalid padding" def test_query_without_manifest(postgres: PostgresContainer): connection_info = _to_connection_info(postgres) diff --git a/wren-core-py/src/context.rs b/wren-core-py/src/context.rs index 0250af9b8..2f0c32e8c 100644 --- a/wren-core-py/src/context.rs +++ b/wren-core-py/src/context.rs @@ -71,8 +71,8 @@ impl PySessionContext { mdl_base64: Option<&str>, remote_functions_path: Option<&str>, ) -> PyResult { - let remote_functions = - Self::read_remote_function_list(remote_functions_path).unwrap(); + let remote_functions = Self::read_remote_function_list(remote_functions_path) + .map_err(CoreError::from)?; let remote_functions: Vec = remote_functions .into_iter() .map(|f| f.into()) @@ -101,7 +101,7 @@ impl PySessionContext { let analyzed_mdl = Arc::new(analyzed_mdl); - let runtime = tokio::runtime::Runtime::new().unwrap(); + let runtime = tokio::runtime::Runtime::new().map_err(CoreError::from)?; let ctx = runtime .block_on(create_ctx_with_mdl(&ctx, Arc::clone(&analyzed_mdl), false)) .map_err(CoreError::from)?; @@ -229,7 +229,7 @@ impl PySessionContext { ); if let Some(path) = path { Ok(csv::Reader::from_path(path) - .unwrap() + .map_err(CoreError::from)? .into_deserialize::() .filter_map(Result::ok) .collect::>()) diff --git a/wren-core-py/src/errors.rs b/wren-core-py/src/errors.rs index 44f1fa2b9..9ad301f85 100644 --- a/wren-core-py/src/errors.rs +++ b/wren-core-py/src/errors.rs @@ -26,30 +26,42 @@ impl From for PyErr { impl From for CoreError { fn from(err: PyErr) -> Self { - CoreError::new(&err.to_string()) + CoreError::new(&format!("PyError: {}", &err)) } } impl From for CoreError { fn from(err: DecodeError) -> Self { - CoreError::new(&err.to_string()) + CoreError::new(&format!("Base64 decode error: {}", err)) } } impl From for CoreError { fn from(err: FromUtf8Error) -> Self { - CoreError::new(&err.to_string()) + CoreError::new(&format!("FromUtf8Error: {}", err)) } } impl From for CoreError { fn from(err: serde_json::Error) -> Self { - CoreError::new(&err.to_string()) + CoreError::new(&format!("Serde JSON error: {}", err)) } } impl From for CoreError { fn from(err: wren_core::DataFusionError) -> Self { - CoreError::new(&err.to_string()) + CoreError::new(&format!("DataFusion error: {}", err)) + } +} + +impl From for CoreError { + fn from(err: csv::Error) -> Self { + CoreError::new(&format!("CSV error: {}", err)) + } +} + +impl From for CoreError { + fn from(err: std::io::Error) -> Self { + CoreError::new(&format!("IO error: {}", err)) } } diff --git a/wren-core/core/src/mdl/builder.rs b/wren-core/core/src/mdl/builder.rs index 2b4058f67..642b52e3a 100644 --- a/wren-core/core/src/mdl/builder.rs +++ b/wren-core/core/src/mdl/builder.rs @@ -3,7 +3,6 @@ use crate::mdl::manifest::{ Column, JoinType, Manifest, Metric, Model, Relationship, TimeGrain, TimeUnit, View, }; -use std::collections::BTreeMap; use std::sync::Arc; /// A builder for creating a Manifest @@ -82,7 +81,6 @@ impl ModelBuilder { primary_key: None, cached: false, refresh_time: None, - properties: BTreeMap::default(), }, } } @@ -122,13 +120,6 @@ impl ModelBuilder { self } - pub fn property(mut self, key: &str, value: &str) -> Self { - self.model - .properties - .insert(key.to_string(), value.to_string()); - self - } - pub fn build(self) -> Arc { Arc::new(self.model) } @@ -148,7 +139,6 @@ impl ColumnBuilder { is_calculated: false, not_null: false, expression: None, - properties: BTreeMap::default(), }, } } @@ -181,13 +171,6 @@ impl ColumnBuilder { self } - pub fn property(mut self, key: &str, value: &str) -> Self { - self.column - .properties - .insert(key.to_string(), value.to_string()); - self - } - pub fn build(self) -> Arc { Arc::new(self.column) } @@ -205,7 +188,6 @@ impl RelationshipBuilder { models: vec![], join_type: JoinType::OneToOne, condition: "".to_string(), - properties: BTreeMap::default(), }, } } @@ -225,13 +207,6 @@ impl RelationshipBuilder { self } - pub fn property(mut self, key: &str, value: &str) -> Self { - self.relationship - .properties - .insert(key.to_string(), value.to_string()); - self - } - pub fn build(self) -> Arc { Arc::new(self.relationship) } @@ -252,7 +227,6 @@ impl MetricBuilder { time_grain: vec![], cached: false, refresh_time: None, - properties: BTreeMap::default(), }, } } @@ -282,13 +256,6 @@ impl MetricBuilder { self } - pub fn property(mut self, key: &str, value: &str) -> Self { - self.metric - .properties - .insert(key.to_string(), value.to_string()); - self - } - pub fn build(self) -> Arc { Arc::new(self.metric) } @@ -334,7 +301,6 @@ impl ViewBuilder { view: View { name: name.to_string(), statement: "".to_string(), - properties: BTreeMap::default(), }, } } @@ -344,13 +310,6 @@ impl ViewBuilder { self } - pub fn property(mut self, key: &str, value: &str) -> Self { - self.view - .properties - .insert(key.to_string(), value.to_string()); - self - } - pub fn build(self) -> Arc { Arc::new(self.view) } @@ -374,7 +333,6 @@ mod test { .calculated(true) .not_null(true) .expression("test") - .property("key", "value") .build(); let json_str = serde_json::to_string(&expected).unwrap(); @@ -430,7 +388,6 @@ mod test { .primary_key("id") .cached(true) .refresh_time("1h") - .property("key", "value") .build(); let json_str = serde_json::to_string(&model).unwrap(); @@ -445,7 +402,6 @@ mod test { .primary_key("id") .cached(true) .refresh_time("1h") - .property("key", "value") .build(); let json_str = serde_json::to_string(&model).unwrap(); @@ -470,7 +426,6 @@ mod test { .model("testB") .join_type(JoinType::OneToMany) .condition("test") - .property("key", "value") .build(); let json_str = serde_json::to_string(&expected).unwrap(); @@ -523,7 +478,6 @@ mod test { ) .cached(true) .refresh_time("1h") - .property("key", "value") .build(); let json_str = serde_json::to_string(&model).unwrap(); @@ -535,7 +489,6 @@ mod test { fn test_view_roundtrip() { let expected = ViewBuilder::new("test") .statement("SELECT * FROM test") - .property("key", "value") .build(); let json_str = serde_json::to_string(&expected).unwrap(); @@ -553,7 +506,6 @@ mod test { .primary_key("id") .cached(true) .refresh_time("1h") - .property("key", "value") .build(); let relationship = RelationshipBuilder::new("test") @@ -561,7 +513,6 @@ mod test { .model("testB") .join_type(JoinType::OneToMany) .condition("test") - .property("key", "value") .build(); let metric = MetricBuilder::new("test") @@ -575,12 +526,10 @@ mod test { ) .cached(true) .refresh_time("1h") - .property("key", "value") .build(); let view = ViewBuilder::new("test") .statement("SELECT * FROM test") - .property("key", "value") .build(); let expected = crate::mdl::builder::ManifestBuilder::new() diff --git a/wren-core/core/src/mdl/manifest.rs b/wren-core/core/src/mdl/manifest.rs index 0cc184beb..98ec42e54 100644 --- a/wren-core/core/src/mdl/manifest.rs +++ b/wren-core/core/src/mdl/manifest.rs @@ -1,4 +1,3 @@ -use std::collections::BTreeMap; use std::fmt::Display; use std::sync::Arc; @@ -39,8 +38,6 @@ pub struct Model { pub cached: bool, #[serde(default)] pub refresh_time: Option, - #[serde(default)] - pub properties: BTreeMap, } impl Model { @@ -170,8 +167,6 @@ pub struct Column { #[serde_as(as = "NoneAsEmptyString")] #[serde(default)] pub expression: Option, - #[serde(default)] - pub properties: BTreeMap, } #[derive(Serialize, Deserialize, Debug, Hash, PartialEq, Eq)] @@ -181,8 +176,6 @@ pub struct Relationship { pub models: Vec, pub join_type: JoinType, pub condition: String, - #[serde(default)] - pub properties: BTreeMap, } #[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Hash, Clone, Copy)] @@ -226,7 +219,6 @@ pub struct Metric { #[serde(default, with = "bool_from_int")] pub cached: bool, pub refresh_time: Option, - pub properties: BTreeMap, } impl Metric { @@ -257,8 +249,6 @@ pub enum TimeUnit { pub struct View { pub name: String, pub statement: String, - #[serde(default)] - pub properties: BTreeMap, } impl View {