From bc49fee92007ab3f3648c1448921725f50196dd7 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Wed, 10 Feb 2021 11:53:10 -0500 Subject: [PATCH] refactor: Remove unneeded StringSetPlan::KnownError variant (#775) * refactor: remove KnownError variant * refactor: rename Known --> KnownOk --- query/src/exec.rs | 26 ++------------- query/src/plan/stringset.rs | 66 +++++-------------------------------- query/src/test.rs | 8 ++--- 3 files changed, 15 insertions(+), 85 deletions(-) diff --git a/query/src/exec.rs b/query/src/exec.rs index 7d1c57adbd..43d3cb1e7e 100644 --- a/query/src/exec.rs +++ b/query/src/exec.rs @@ -193,8 +193,7 @@ impl Executor { /// Executes this plan and returns the resulting set of strings pub async fn to_string_set(&self, plan: StringSetPlan) -> Result { match plan { - StringSetPlan::KnownOk(ss) => Ok(ss), - StringSetPlan::KnownError(source) => Err(Error::Execution { source }), + StringSetPlan::Known(ss) => Ok(ss), StringSetPlan::Plan(plans) => self .run_logical_plans(plans) .await? @@ -415,8 +414,7 @@ mod tests { #[tokio::test] async fn executor_known_string_set_plan_ok() -> Result<()> { let expected_strings = to_set(&["Foo", "Bar"]); - let result: Result<_> = Ok(expected_strings.clone()); - let plan = result.into(); + let plan = StringSetPlan::Known(expected_strings.clone()); let executor = Executor::default(); let result_strings = executor.to_string_set(plan).await?; @@ -424,26 +422,6 @@ mod tests { Ok(()) } - #[tokio::test] - async fn executor_known_string_set_plan_err() -> Result<()> { - let result = InternalResultsExtraction { - message: "this is a test", - } - .fail(); - - let plan = result.into(); - - let executor = Executor::default(); - let actual_result = executor.to_string_set(plan).await; - assert!(actual_result.is_err()); - assert!( - format!("{:?}", actual_result).contains("this is a test"), - "Actual result: '{:?}'", - actual_result - ); - Ok(()) - } - #[tokio::test] async fn executor_datafusion_string_set_single_plan_no_batches() -> Result<()> { // Test with a single plan that produces no batches diff --git a/query/src/plan/stringset.rs b/query/src/plan/stringset.rs index e7ef6a58e0..584c17b0f4 100644 --- a/query/src/plan/stringset.rs +++ b/query/src/plan/stringset.rs @@ -30,12 +30,9 @@ pub type Result = std::result::Result; /// well a variant that runs a full on DataFusion plan. #[derive(Debug)] pub enum StringSetPlan { - /// The plan errored and we are delaying reporting the results - KnownError(Box), - /// The results are known from metadata only without having to run /// an actual datafusion plan - KnownOk(StringSetRef), + Known(StringSetRef), /// A DataFusion plan(s) to execute. Each plan must produce /// RecordBatches with exactly one String column, though the @@ -50,7 +47,7 @@ pub enum StringSetPlan { impl From for StringSetPlan { /// Create a StringSetPlan from a StringSetRef fn from(set: StringSetRef) -> Self { - Self::KnownOk(set) + Self::Known(set) } } @@ -58,21 +55,7 @@ impl From for StringSetPlan { /// Create a StringSetPlan from a StringSet result, wrapping the error type /// appropriately fn from(set: StringSet) -> Self { - Self::KnownOk(StringSetRef::new(set)) - } -} - -impl From> for StringSetPlan -where - E: std::error::Error + Send + Sync + 'static, -{ - /// Create a `StringSetPlan` from a `Result`, wrapping the - /// error type appropriately - fn from(result: Result) -> Self { - match result { - Ok(set) => Self::KnownOk(set), - Err(e) => Self::KnownError(Box::new(e)), - } + Self::Known(StringSetRef::new(set)) } } @@ -91,9 +74,6 @@ impl From> for StringSetPlan { /// strings, otherwise it falls back to generic plans #[derive(Debug, Default)] pub struct StringSetPlanBuilder { - /// If there was any error - error: Option>, - /// Known strings strings: StringSet, /// General plans @@ -115,8 +95,7 @@ impl StringSetPlanBuilder { /// passes on the plan pub fn append(mut self, other: StringSetPlan) -> Self { match other { - StringSetPlan::KnownError(err) => self.error = Some(err), - StringSetPlan::KnownOk(ssref) => match Arc::try_unwrap(ssref) { + StringSetPlan::Known(ssref) => match Arc::try_unwrap(ssref) { Ok(mut ss) => { self.strings.append(&mut ss); } @@ -137,18 +116,11 @@ impl StringSetPlanBuilder { /// Create a StringSetPlan that produces the deduplicated (union) /// of all plans `append`ed to this builder. pub fn build(self) -> Result { - let Self { - error, - strings, - mut plans, - } = self; + let Self { strings, mut plans } = self; - if let Some(err) = error { - // is an error, so nothing else matters - Ok(StringSetPlan::KnownError(err)) - } else if plans.is_empty() { + if plans.is_empty() { // only a known set of strings - Ok(StringSetPlan::KnownOk(Arc::new(strings))) + Ok(StringSetPlan::Known(Arc::new(strings))) } else { // Had at least one general plan, so need to use general // purpose plan for the known strings @@ -177,7 +149,7 @@ mod tests { fn test_builder_empty() { let plan = StringSetPlanBuilder::new().build().unwrap(); let empty_ss = StringSet::new().into(); - if let StringSetPlan::KnownOk(ss) = plan { + if let StringSetPlan::Known(ss) = plan { assert_eq!(ss, empty_ss) } else { panic!("unexpected type: {:?}", plan) @@ -194,7 +166,7 @@ mod tests { let expected_ss = to_string_set(&["foo", "bar", "baz"]).into(); - if let StringSetPlan::KnownOk(ss) = plan { + if let StringSetPlan::Known(ss) = plan { assert_eq!(ss, expected_ss) } else { panic!("unexpected type: {:?}", plan) @@ -212,26 +184,6 @@ mod tests { impl std::error::Error for TestError {} - #[test] - fn test_builder_error() { - let err = Box::new(TestError {}); - let error = StringSetPlan::KnownError(err); - - // when an error is generated, ensure it gets returned - let plan = StringSetPlanBuilder::new() - .append(to_string_set(&["foo", "bar"]).into()) - .append(error) - .append(to_string_set(&["bar", "baz"]).into()) - .build() - .unwrap(); - - if let StringSetPlan::KnownError(err) = plan { - assert_eq!(err.to_string(), "this is an error") - } else { - panic!("unexpected type: {:?}", plan) - } - } - #[tokio::test] async fn test_builder_plan() { let batch = str_iter_to_batch("column_name", vec![Some("from_a_plan")]).unwrap(); diff --git a/query/src/test.rs b/query/src/test.rs index f3dbe9eb4b..f775f93154 100644 --- a/query/src/test.rs +++ b/query/src/test.rs @@ -352,9 +352,9 @@ impl Database for TestDatabase { // Turn None into an error .context(General { message: "No saved column_names in TestDatabase", - }); + })?; - Ok(column_names.into()) + Ok(StringSetPlan::Known(column_names)) } async fn field_column_names(&self, predicate: Predicate) -> Result { @@ -411,9 +411,9 @@ impl Database for TestDatabase { // Turn None into an error .context(General { message: "No saved column_values in TestDatabase", - }); + })?; - Ok(column_values.into()) + Ok(StringSetPlan::Known(column_values)) } async fn query_series(&self, predicate: Predicate) -> Result {