feat(core): remove properties field from mdl and improve the error me…

…ssage (#862)
Canner · Oct 30, 2024 · 3ed5e2a · 3ed5e2a
1 parent 282c953
commit 3ed5e2a
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 71 deletions.
diff --git 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
@@ -71,8 +71,8 @@ impl PySessionContext {
         mdl_base64: Option<&str>,
         remote_functions_path: Option<&str>,
     ) -> PyResult<Self> {
-        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<RemoteFunction> = 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::<PyRemoteFunction>()
                 .filter_map(Result::ok)
                 .collect::<Vec<_>>())

diff --git a/wren-core-py/src/errors.rs b/wren-core-py/src/errors.rs
@@ -26,30 +26,42 @@ impl From<CoreError> for PyErr {
 
 impl From<PyErr> for CoreError {
     fn from(err: PyErr) -> Self {
-        CoreError::new(&err.to_string())
+        CoreError::new(&format!("PyError: {}", &err))
     }
 }
 
 impl From<DecodeError> for CoreError {
     fn from(err: DecodeError) -> Self {
-        CoreError::new(&err.to_string())
+        CoreError::new(&format!("Base64 decode error: {}", err))
     }
 }
 
 impl From<FromUtf8Error> for CoreError {
     fn from(err: FromUtf8Error) -> Self {
-        CoreError::new(&err.to_string())
+        CoreError::new(&format!("FromUtf8Error: {}", err))
     }
 }
 
 impl From<serde_json::Error> for CoreError {
     fn from(err: serde_json::Error) -> Self {
-        CoreError::new(&err.to_string())
+        CoreError::new(&format!("Serde JSON error: {}", err))
     }
 }
 
 impl From<wren_core::DataFusionError> for CoreError {
     fn from(err: wren_core::DataFusionError) -> Self {
-        CoreError::new(&err.to_string())
+        CoreError::new(&format!("DataFusion error: {}", err))
+    }
+}
+
+impl From<csv::Error> for CoreError {
+    fn from(err: csv::Error) -> Self {
+        CoreError::new(&format!("CSV error: {}", err))
+    }
+}
+
+impl From<std::io::Error> 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
@@ -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<Model> {
         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<Column> {
         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<Relationship> {
         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<Metric> {
         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<View> {
         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,15 +506,13 @@ mod test {
             .primary_key("id")
             .cached(true)
             .refresh_time("1h")
-            .property("key", "value")
             .build();
 
         let relationship = RelationshipBuilder::new("test")
             .model("testA")
             .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
@@ -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<String>,
-    #[serde(default)]
-    pub properties: BTreeMap<String, String>,
 }
 
 impl Model {
@@ -170,8 +167,6 @@ pub struct Column {
     #[serde_as(as = "NoneAsEmptyString")]
     #[serde(default)]
     pub expression: Option<String>,
-    #[serde(default)]
-    pub properties: BTreeMap<String, String>,
 }
 
 #[derive(Serialize, Deserialize, Debug, Hash, PartialEq, Eq)]
@@ -181,8 +176,6 @@ pub struct Relationship {
     pub models: Vec<String>,
     pub join_type: JoinType,
     pub condition: String,
-    #[serde(default)]
-    pub properties: BTreeMap<String, String>,
 }
 
 #[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<String>,
-    pub properties: BTreeMap<String, String>,
 }
 
 impl Metric {
@@ -257,8 +249,6 @@ pub enum TimeUnit {
 pub struct View {
     pub name: String,
     pub statement: String,
-    #[serde(default)]
-    pub properties: BTreeMap<String, String>,
 }
 
 impl View {