From fcaf189697bce99a51245b8518cb30f481172446 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Mon, 20 Feb 2023 19:34:03 -0800 Subject: [PATCH 1/3] refactor: Attempt to replace our `IntoOnly` with `ExactlyOne` Since I originally wrote this (and others have iterated on it), Itertools released `ExactlyOne`, which has better errors and reduces our custom code. Unfortunately, I couldn't fix a rust type error, and spent too long on it already. So pushing what I have in case anyone wants to take a look. There's also a decent chance that we replace the `parser.rs` code, in which case this type error becomes moot, and we can merge this anyway. --- prql-compiler/src/error.rs | 11 +++------ prql-compiler/src/semantic/resolver.rs | 4 +-- prql-compiler/src/sql/gen_query.rs | 6 +++-- prql-compiler/src/utils/only.rs | 34 +++----------------------- 4 files changed, 13 insertions(+), 42 deletions(-) diff --git a/prql-compiler/src/error.rs b/prql-compiler/src/error.rs index 92f3f3492210..b82f937eeab0 100644 --- a/prql-compiler/src/error.rs +++ b/prql-compiler/src/error.rs @@ -1,13 +1,12 @@ pub use anyhow::Result; use ariadne::{Cache, Config, Label, Report, ReportKind, Source}; +use itertools::Itertools; use serde::{Deserialize, Serialize}; use std::error::Error as StdError; use std::fmt::{self, Debug, Display, Formatter}; use std::ops::{Add, Range}; -use crate::utils::IntoOnly; - #[derive(Clone, PartialEq, Eq, Copy, Serialize, Deserialize)] pub struct Span { pub start: usize, @@ -216,13 +215,9 @@ impl ErrorMessages { } self } -} - -impl IntoOnly for ErrorMessages { - type Item = ErrorMessage; - fn into_only(self) -> Result { - self.inner.into_only() + fn exactly_one(self) -> Result { + Ok(self.inner.into_iter().exactly_one()?) } } diff --git a/prql-compiler/src/semantic/resolver.rs b/prql-compiler/src/semantic/resolver.rs index d4c4bcc0e948..f15003f4e3c5 100644 --- a/prql-compiler/src/semantic/resolver.rs +++ b/prql-compiler/src/semantic/resolver.rs @@ -760,15 +760,15 @@ fn env_of_closure(closure: Closure) -> (Module, Expr) { mod test { use anyhow::Result; use insta::assert_yaml_snapshot; + use itertools::Itertools; use crate::ast::pl::{Expr, Ty}; use crate::semantic::resolve_only; - use crate::utils::IntoOnly; fn parse_and_resolve(query: &str) -> Result { let (stmts, _) = resolve_only(crate::parser::parse(query)?, None)?; - Ok(*stmts.into_only()?.kind.into_main()?) + Ok(*stmts.into_iter().exactly_one()?.kind.into_main()?) } fn resolve_type(query: &str) -> Result { diff --git a/prql-compiler/src/sql/gen_query.rs b/prql-compiler/src/sql/gen_query.rs index fba04d0be692..421ce63d3170 100644 --- a/prql-compiler/src/sql/gen_query.rs +++ b/prql-compiler/src/sql/gen_query.rs @@ -16,7 +16,8 @@ use crate::ast::pl::{BinOp, JoinSide, Literal, RelationLiteral}; use crate::ast::rq::{CId, Expr, ExprKind, Query, RelationKind, TableRef, Transform}; use crate::sql::anchor::anchor_split; use crate::sql::preprocess::SqlRelationKind; -use crate::utils::{BreakUp, IntoOnly, Pluck}; +use crate::utils::{BreakUp, Pluck}; + use crate::Target; use super::context::AnchorContext; @@ -249,7 +250,8 @@ fn sql_select_query_of_pipeline( let projection = pipeline .pluck(|t| t.into_super_and(|t| t.into_select())) - .into_only() + .into_iter() + .exactly_one() .unwrap(); let projection = translate_wildcards(&ctx.anchor, projection); let projection = translate_select_items(projection.0, projection.1, ctx)?; diff --git a/prql-compiler/src/utils/only.rs b/prql-compiler/src/utils/only.rs index 1a52e97027ee..ca8708349c8a 100644 --- a/prql-compiler/src/utils/only.rs +++ b/prql-compiler/src/utils/only.rs @@ -1,46 +1,20 @@ use anyhow::{anyhow, Result}; -use itertools::{Itertools, Position}; use crate::ast::pl::Expr; use crate::error::{Error, Reason}; -// Inspired by version in sqlparser-rs; I'm surprised there isn't a version in -// the stdlib / Itertools. -/// Returns the only element of an Iterator, or an error if it has more than one element. pub trait IntoOnly { type Item; fn into_only(self) -> Result; } -impl IntoOnly for I -where - I: IntoIterator, - // See below re using Debug. - // I: std::fmt::Debug, - // ::IntoIter: std::fmt::Debug, -{ - type Item = T; - - fn into_only(self) -> Result { - match self.into_iter().with_position().next() { - Some(Position::Only(item)) => Ok(item), - // Can't get the debug of the iterator because it's already - // consumed; is there a way around this? I guess we could show - // the items after the second, which is kinda weird. - Some(Position::First(_)) => Err(anyhow!("Expected only one element, but found more.",)), - None => Err(anyhow!("Expected one element, but found none.",)), - _ => unreachable!(), - } - } -} - -pub trait IntoOnlyNode { - fn into_only_node(self, who: &str, occupation: &str) -> Result; +pub trait ExactlyOneNode { + fn exactly_one_node(self, who: &str, occupation: &str) -> Result; } -impl IntoOnlyNode for Vec { - fn into_only_node(mut self, who: &str, occupation: &str) -> Result { +impl ExactlyOneNode for Vec { + fn exactly_one_node(mut self, who: &str, occupation: &str) -> Result { match self.len() { 1 => Ok(self.remove(0)), 0 => Err(Error { From ceccaee39ea5d40c680119e8c3d2fe81513bd7bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alja=C5=BE=20Mur=20Er=C5=BEen?= Date: Mon, 27 Feb 2023 22:09:33 +0100 Subject: [PATCH 2/3] remove IntoOnly completely - even from public API --- prql-compiler/src/error.rs | 5 ----- prql-compiler/src/lib.rs | 1 - prql-compiler/src/utils/mod.rs | 2 +- prql-compiler/src/utils/only.rs | 25 +------------------------ 4 files changed, 2 insertions(+), 31 deletions(-) diff --git a/prql-compiler/src/error.rs b/prql-compiler/src/error.rs index b82f937eeab0..7861baeab3fd 100644 --- a/prql-compiler/src/error.rs +++ b/prql-compiler/src/error.rs @@ -1,7 +1,6 @@ pub use anyhow::Result; use ariadne::{Cache, Config, Label, Report, ReportKind, Source}; -use itertools::Itertools; use serde::{Deserialize, Serialize}; use std::error::Error as StdError; use std::fmt::{self, Debug, Display, Formatter}; @@ -215,10 +214,6 @@ impl ErrorMessages { } self } - - fn exactly_one(self) -> Result { - Ok(self.inner.into_iter().exactly_one()?) - } } impl ErrorMessage { diff --git a/prql-compiler/src/lib.rs b/prql-compiler/src/lib.rs index 18894b6b5348..7107f45baaa6 100644 --- a/prql-compiler/src/lib.rs +++ b/prql-compiler/src/lib.rs @@ -87,7 +87,6 @@ mod test; mod utils; pub use error::{downcast, Error, ErrorMessage, ErrorMessages, Reason, SourceLocation, Span}; -pub use utils::IntoOnly; use once_cell::sync::Lazy; use semver::Version; diff --git a/prql-compiler/src/utils/mod.rs b/prql-compiler/src/utils/mod.rs index 0ff61841b1b1..792d55fbdf64 100644 --- a/prql-compiler/src/utils/mod.rs +++ b/prql-compiler/src/utils/mod.rs @@ -3,11 +3,11 @@ mod only; mod toposort; pub use id_gen::{IdGenerator, NameGenerator}; -use itertools::Itertools; pub use only::*; pub use toposort::toposort; use anyhow::Result; +use itertools::Itertools; pub trait OrMap { /// Merges two options into one using `f`. diff --git a/prql-compiler/src/utils/only.rs b/prql-compiler/src/utils/only.rs index ca8708349c8a..0eef8df16664 100644 --- a/prql-compiler/src/utils/only.rs +++ b/prql-compiler/src/utils/only.rs @@ -1,14 +1,8 @@ -use anyhow::{anyhow, Result}; +use anyhow::Result; use crate::ast::pl::Expr; use crate::error::{Error, Reason}; -pub trait IntoOnly { - type Item; - - fn into_only(self) -> Result; -} - pub trait ExactlyOneNode { fn exactly_one_node(self, who: &str, occupation: &str) -> Result; } @@ -38,20 +32,3 @@ impl ExactlyOneNode for Vec { } } } - -pub trait Only { - fn only(&self) -> Result<&T>; -} - -impl Only for [T] -where - T: std::fmt::Debug, -{ - fn only(&self) -> Result<&T> { - if self.len() == 1 { - Ok(self.first().unwrap()) - } else { - Err(anyhow!("Expected 1 item, got {}; {:?}", self.len(), self)) - } - } -} From 1c5bdd5af1f2e9ef1ec9ad6cea6e4b869721d0cc Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Mon, 27 Feb 2023 13:27:46 -0800 Subject: [PATCH 3/3] Allow multiple ErrorMessages in prql-python --- prql-python/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/prql-python/src/lib.rs b/prql-python/src/lib.rs index 1202df209d82..6beebe154a69 100644 --- a/prql-python/src/lib.rs +++ b/prql-python/src/lib.rs @@ -1,7 +1,7 @@ #![cfg(not(target_family = "wasm"))] use std::str::FromStr; -use prql_compiler::{self, IntoOnly, Target}; +use prql_compiler::{self, Target}; use pyo3::{exceptions, prelude::*}; #[pyfunction] @@ -11,7 +11,7 @@ pub fn compile(prql_query: &str, options: Option) -> PyResult(e.into_only().unwrap().reason))) + .map_err(|e| (PyErr::new::(e.to_string()))) } #[pyfunction]