Skip to content

Commit

Permalink
refactor: Attempt to replace our IntoOnly with ExactlyOne (#1915)
Browse files Browse the repository at this point in the history
* 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.

* remove IntoOnly completely - even from public API

* Allow multiple ErrorMessages in prql-python

---------

Co-authored-by: Aljaž Mur Eržen <aljaz.erzen@gmail.com>
  • Loading branch information
max-sixty and aljazerzen authored Feb 28, 2023
1 parent 7fce2b3 commit 63b01fa
Show file tree
Hide file tree
Showing 7 changed files with 14 additions and 72 deletions.
10 changes: 0 additions & 10 deletions prql-compiler/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ 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,
Expand Down Expand Up @@ -218,14 +216,6 @@ impl ErrorMessages {
}
}

impl IntoOnly for ErrorMessages {
type Item = ErrorMessage;

fn into_only(self) -> Result<Self::Item> {
self.inner.into_only()
}
}

impl ErrorMessage {
fn compose_display<'a, C>(&self, source_id: &'a str, cache: C, color: bool) -> Option<String>
where
Expand Down
1 change: 0 additions & 1 deletion prql-compiler/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions prql-compiler/src/semantic/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Expr> {
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<Ty> {
Expand Down
6 changes: 4 additions & 2 deletions prql-compiler/src/sql/gen_query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)?;
Expand Down
2 changes: 1 addition & 1 deletion prql-compiler/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> {
/// Merges two options into one using `f`.
Expand Down
59 changes: 5 additions & 54 deletions prql-compiler/src/utils/only.rs
Original file line number Diff line number Diff line change
@@ -1,46 +1,14 @@
use anyhow::{anyhow, Result};
use itertools::{Itertools, Position};
use anyhow::Result;

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<Self::Item>;
}

impl<T, I> IntoOnly for I
where
I: IntoIterator<Item = T>,
// See below re using Debug.
// I: std::fmt::Debug,
// <I as IntoIterator>::IntoIter: std::fmt::Debug,
{
type Item = T;

fn into_only(self) -> Result<T> {
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<Expr, Error>;
pub trait ExactlyOneNode {
fn exactly_one_node(self, who: &str, occupation: &str) -> Result<Expr, Error>;
}

impl IntoOnlyNode for Vec<Expr> {
fn into_only_node(mut self, who: &str, occupation: &str) -> Result<Expr, Error> {
impl ExactlyOneNode for Vec<Expr> {
fn exactly_one_node(mut self, who: &str, occupation: &str) -> Result<Expr, Error> {
match self.len() {
1 => Ok(self.remove(0)),
0 => Err(Error {
Expand All @@ -64,20 +32,3 @@ impl IntoOnlyNode for Vec<Expr> {
}
}
}

pub trait Only<T> {
fn only(&self) -> Result<&T>;
}

impl<T> Only<T> 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))
}
}
}
4 changes: 2 additions & 2 deletions prql-python/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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]
Expand All @@ -11,7 +11,7 @@ pub fn compile(prql_query: &str, options: Option<CompileOptions>) -> PyResult<St
.and_then(prql_compiler::pl_to_rq)
.and_then(|rq| prql_compiler::rq_to_sql(rq, &options.map(|o| o.into()).unwrap_or_default()))
.map_err(|e| e.composed("", prql_query, false))
.map_err(|e| (PyErr::new::<exceptions::PySyntaxError, _>(e.into_only().unwrap().reason)))
.map_err(|e| (PyErr::new::<exceptions::PySyntaxError, _>(e.to_string())))
}

#[pyfunction]
Expand Down

0 comments on commit 63b01fa

Please sign in to comment.