Skip to content

Commit

Permalink
lower literals when binding eval
Browse files Browse the repository at this point in the history
  • Loading branch information
@jpschorr
jpschorr committed Dec 5, 2024
1 parent 1cc34a4 commit 8921cc7
Show file tree
Hide file tree
Showing 10 changed files with 85 additions and 64 deletions.
2 changes: 1 addition & 1 deletion extension/partiql-extension-ion/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ unicase = "2.7"
rust_decimal = { version = "1.36.0", default-features = false, features = ["std"] }
rust_decimal_macros = "1.36"
ion-rs_old = { version = "0.18", package = "ion-rs" }
ion-rs = { features = ["experimental"], git = "https://github.com/amazon-ion/ion-rust.git", branch = "feat-owned-conversion-qol" }
ion-rs = { features = ["experimental"], git = "https://github.com/amazon-ion/ion-rust.git", branch = "main" }

time = { version = "0.3", features = ["macros"] }
once_cell = "1"
Expand Down
38 changes: 17 additions & 21 deletions extension/partiql-extension-ion/src/boxed_ion.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use ion_rs::{
use ion_rs_old::IonReader;

Check warning on line 5 in extension/partiql-extension-ion/src/boxed_ion.rs

View workflow job for this annotation

GitHub Actions / clippy

unused import: `ion_rs_old::IonReader` 

warning: unused import: `ion_rs_old::IonReader`
 --> extension/partiql-extension-ion/src/boxed_ion.rs:5:5
  |
5 | use ion_rs_old::IonReader;
  |     ^^^^^^^^^^^^^^^^^^^^^
use partiql_value::datum::{
Datum, DatumCategoryOwned, DatumCategoryRef, DatumSeqOwned, DatumSeqRef, DatumTupleOwned,
DatumTupleRef, OwnedSequenceView, OwnedTupleView, RefSequenceView, RefTupleView, SequenceDatum,
TupleDatum,
DatumTupleRef, DatumValueOwned, DatumValueRef, OwnedSequenceView, OwnedTupleView,
RefSequenceView, RefTupleView, SequenceDatum, TupleDatum,
};
use partiql_value::embedded_document::{
EmbeddedDocResult, EmbeddedDocValueIntoIterator, EmbeddedDocument, EmbeddedDocumentType,
Expand Down Expand Up @@ -134,7 +134,7 @@ impl EmbeddedDocument for BoxedIon {
IonType::SExp => DatumCategoryRef::Sequence(DatumSeqRef::Dynamic(self)),
IonType::Null => DatumCategoryRef::Null,
IonType::Struct => DatumCategoryRef::Tuple(DatumTupleRef::Dynamic(self)),
_ => DatumCategoryRef::Scalar(todo!()),
_ => DatumCategoryRef::Scalar(DatumValueRef::Value(todo!())),

Check warning on line 137 in extension/partiql-extension-ion/src/boxed_ion.rs

View workflow job for this annotation

GitHub Actions / clippy

unreachable call 

warning: unreachable call
   --> extension/partiql-extension-ion/src/boxed_ion.rs:137:47
    |
137 |                 _ => DatumCategoryRef::Scalar(DatumValueRef::Value(todo!())),
    |                                               ^^^^^^^^^^^^^^^^^^^^ ------- any code following this expression is unreachable
    |                                               |
    |                                               unreachable call
    |
    = note: `#[warn(unreachable_code)]` on by default
},
}
}
Expand All @@ -150,7 +150,7 @@ impl EmbeddedDocument for BoxedIon {
IonType::SExp => DatumCategoryOwned::Sequence(DatumSeqOwned::Dynamic(self)),
IonType::Null => DatumCategoryOwned::Null,
IonType::Struct => DatumCategoryOwned::Tuple(DatumTupleOwned::Dynamic(self)),
_ => DatumCategoryOwned::Scalar(todo!()),
_ => DatumCategoryOwned::Scalar(DatumValueOwned::Value(self.into_value())),
},
}
}
Expand All @@ -166,7 +166,7 @@ impl SequenceDatum for BoxedIon {
BoxedIonValue::Stream() => {
todo!()
}
BoxedIonValue::Sequence(seq) => seq.size_hint().0, // TODO
BoxedIonValue::Sequence(seq) => seq.len(),
BoxedIonValue::Value(elt) => match elt.expect_sequence() {
Ok(seq) => seq.len(), // TODO
Err(e) => todo!(),

Check warning on line 172 in extension/partiql-extension-ion/src/boxed_ion.rs

View workflow job for this annotation

GitHub Actions / clippy

unused variable: `e` 

warning: unused variable: `e`
   --> extension/partiql-extension-ion/src/boxed_ion.rs:172:21
    |
172 |                 Err(e) => todo!(),
    |                     ^ help: if this is intentional, prefix it with an underscore: `_e`
Expand All @@ -182,9 +182,8 @@ impl<'a> RefSequenceView<'a, Value> for BoxedIon {
todo!()
}
BoxedIonValue::Sequence(seq) => seq
.clone() // TODO remove clone by holding vecdeque directly?
.nth(k as usize)
.map(|elt| Cow::Owned(self.child_value(elt))),
.get(k as usize)
.map(|elt| Cow::Owned(self.child_value(elt.clone()))), // TODO remove clone
BoxedIonValue::Value(elt) => match elt.expect_sequence() {
Ok(seq) => seq
.iter()
Expand All @@ -203,9 +202,10 @@ impl OwnedSequenceView<Value> for BoxedIon {
BoxedIonValue::Stream() => {
todo!()
}
BoxedIonValue::Sequence(mut seq) => {
seq.nth(k as usize).map(|elt| Self::new_value(elt, ctx))
}
BoxedIonValue::Sequence(seq) => seq
.into_iter()
.nth(k as usize)
.map(|elt| Self::new_value(elt, ctx)),
BoxedIonValue::Value(elt) => match elt.try_into_sequence() {
Ok(seq) => seq
.into_iter()
Expand Down Expand Up @@ -379,7 +379,7 @@ impl BoxedIon {

match elt.try_into_sequence() {
Err(err) => BoxedIonValue::Value(err.original_value()),
Ok(seq) => BoxedIonValue::Sequence(seq.into_iter()),
Ok(seq) => BoxedIonValue::Sequence(seq),
}
}
}
Expand Down Expand Up @@ -418,7 +418,7 @@ enum BoxedIonStreamType {
enum BoxedIonValue {
Stream(),
Value(Element),
Sequence(OwnedSequenceIterator),
Sequence(Sequence),
}

impl From<Element> for BoxedIonValue {
Expand All @@ -427,8 +427,8 @@ impl From<Element> for BoxedIonValue {
}
}

impl From<OwnedSequenceIterator> for BoxedIonValue {
fn from(value: OwnedSequenceIterator) -> Self {
impl From<Sequence> for BoxedIonValue {
fn from(value: Sequence) -> Self {
BoxedIonValue::Sequence(value)
}
}
Expand All @@ -441,9 +441,7 @@ impl Clone for BoxedIonValue {
todo!("stream not cloneable? ")
}
BoxedIonValue::Value(val) => BoxedIonValue::Value(val.clone()),
BoxedIonValue::Sequence(seq) => {
todo!("clone for Seq")
}
BoxedIonValue::Sequence(seq) => BoxedIonValue::Sequence(seq.clone()),
}
}
}
Expand All @@ -456,9 +454,7 @@ impl Display for BoxedIonValue {
todo!("stream not displayable? ")
}
BoxedIonValue::Value(val) => std::fmt::Display::fmt(val, f),
BoxedIonValue::Sequence(seq) => {
todo!("display for Seq")
}
BoxedIonValue::Sequence(seq) => std::fmt::Debug::fmt(&seq, f),
}
}
}
Expand Down
39 changes: 22 additions & 17 deletions partiql-eval/src/eval/eval_expr_wrapper.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::eval::expr::{BindError, EvalExpr};
use crate::eval::EvalContext;
use itertools::Itertools;

use partiql_types::{type_dynamic, PartiqlShape, Static, StaticCategory, StaticType, TYPE_DYNAMIC};
use partiql_types::{type_dynamic, PartiqlShape, Static, StaticCategory, TYPE_DYNAMIC};
use partiql_value::Value::{Missing, Null};
use partiql_value::{Tuple, Value};

Expand All @@ -14,7 +14,7 @@ use std::hash::Hash;

use std::marker::PhantomData;

use partiql_value::datum::{Datum, DatumCategory, DatumCategoryRef, DatumValueRef};
use partiql_value::datum::{DatumCategory, DatumCategoryRef, DatumValueRef};
use std::ops::ControlFlow;

trait TypeSatisfier {
Expand All @@ -28,21 +28,26 @@ impl TypeSatisfier for Static {
(_, DatumCategoryRef::Null) => true,
(_, DatumCategoryRef::Missing) => true,
(StaticCategory::Scalar(ty), DatumCategoryRef::Scalar(scalar)) => match scalar {
DatumValueRef::Value(scalar) => match (ty, scalar) {
(
Static::Int | Static::Int8 | Static::Int16 | Static::Int32 | Static::Int64,
Value::Integer(_),
) => true,
(Static::Bool, Value::Boolean(_)) => true,
(Static::Decimal | Static::DecimalP(_, _), Value::Decimal(_)) => true,
(Static::Float32 | Static::Float64, Value::Real(_)) => true,
(
Static::String | Static::StringFixed(_) | Static::StringVarying(_),
Value::String(_),
) => true,
(Static::DateTime, Value::DateTime(_)) => true,
_ => false,
},
DatumValueRef::Value(scalar) => {
matches!(
(ty, scalar),
(
Static::Int
| Static::Int8
| Static::Int16
| Static::Int32
| Static::Int64,
Value::Integer(_),
) | (Static::Bool, Value::Boolean(_))
| (Static::Decimal | Static::DecimalP(_, _), Value::Decimal(_))
| (Static::Float32 | Static::Float64, Value::Real(_))
| (
Static::String | Static::StringFixed(_) | Static::StringVarying(_),
Value::String(_),
)
| (Static::DateTime, Value::DateTime(_))
)
}
},
(StaticCategory::Sequence(shape), DatumCategoryRef::Sequence(seq)) => {

Check warning on line 52 in partiql-eval/src/eval/eval_expr_wrapper.rs

View workflow job for this annotation

GitHub Actions / clippy

unused variable: `seq` 

warning: unused variable: `seq`
  --> partiql-eval/src/eval/eval_expr_wrapper.rs:52:74
   |
52 |             (StaticCategory::Sequence(shape), DatumCategoryRef::Sequence(seq)) => {
   |                                                                          ^^^ help: if this is intentional, prefix it with an underscore: `_seq`

Check warning on line 52 in partiql-eval/src/eval/eval_expr_wrapper.rs

View workflow job for this annotation

GitHub Actions / clippy

unused variable: `shape` 

warning: unused variable: `shape`
  --> partiql-eval/src/eval/eval_expr_wrapper.rs:52:39
   |
52 |             (StaticCategory::Sequence(shape), DatumCategoryRef::Sequence(seq)) => {
   |                                       ^^^^^ help: if this is intentional, prefix it with an underscore: `_shape`
   |
   = note: `#[warn(unused_variables)]` on by default
//shape.all_satisfies(seq)
Expand Down
6 changes: 5 additions & 1 deletion partiql-eval/src/eval/expr/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub(crate) use operators::*;

use crate::eval::EvalContext;

use partiql_value::datum::DatumLowerError;
use partiql_value::{Tuple, Value};
use std::borrow::Cow;
use std::fmt::Debug;
Expand All @@ -44,7 +45,7 @@ pub trait EvalExpr: Debug {
}
}

#[derive(Error, Debug, Clone, PartialEq)]
#[derive(Error, Debug)]
#[non_exhaustive]
/// An error in binding an expression for evaluation
pub enum BindError {
Expand All @@ -59,6 +60,9 @@ pub enum BindError {
#[error("Not yet implemented: {0}")]
NotYetImplemented(String),

#[error("Error lowering literal value: {0}")]
LiteralValue(#[from] DatumLowerError),

/// Any other error.
#[error("Bind error: unknown error")]
Unknown,
Expand Down
18 changes: 14 additions & 4 deletions partiql-eval/src/eval/expr/operators.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,22 @@ use std::ops::ControlFlow;
/// Represents a literal in (sub)query, e.g. `1` in `a + 1`.
#[derive(Clone)]
pub(crate) struct EvalLitExpr {
pub(crate) lit: Value,
pub(crate) val: Value,
}

impl EvalLitExpr {
pub(crate) fn new(val: Value) -> Self {
Self { val }
}

fn lower(&self) -> DatumLowerResult<EvalLitExpr> {
self.val.clone().into_lower().map(Self::new)
}
}

impl Debug for EvalLitExpr {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
self.lit.fmt(f)
self.val.fmt(f)
}
}

Expand All @@ -41,7 +51,7 @@ impl BindEvalExpr for EvalLitExpr {
self,
_args: Vec<Box<dyn EvalExpr>>,
) -> Result<Box<dyn EvalExpr>, BindError> {
Ok(Box::new(self.clone()))
Ok(Box::new(self.lower()?))
}
}

Expand All @@ -54,7 +64,7 @@ impl EvalExpr for EvalLitExpr {
where
'c: 'a,
{
Cow::Borrowed(&self.lit)
Cow::Borrowed(&self.val)
}
}

Expand Down
12 changes: 6 additions & 6 deletions partiql-eval/src/plan.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,9 @@ impl<'c> EvaluatorPlanner<'c> {
}
BindError::NotYetImplemented(name) => PlanningError::NotYetImplemented(name),
BindError::ArgumentConstraint(msg) => PlanningError::IllegalState(msg),
BindError::LiteralValue(err) => {
PlanningError::IllegalState(format!("Literal error: {}", err))
}
};

self.err(err)
Expand Down Expand Up @@ -391,10 +394,7 @@ impl<'c> EvaluatorPlanner<'c> {
),
ValueExpr::Lit(lit) => (
"literal",
EvalLitExpr {
lit: Value::from(lit.as_ref().clone()),
}
.bind::<{ STRICT }>(vec![]),
EvalLitExpr::new(Value::from(lit.as_ref().clone())).bind::<{ STRICT }>(vec![]),
),
ValueExpr::Path(expr, components) => (
"path",
Expand Down Expand Up @@ -514,7 +514,7 @@ impl<'c> EvaluatorPlanner<'c> {
// If no `ELSE` clause is specified, use implicit `ELSE NULL` (see section 6.9, pg 142 of SQL-92 spec)
None => self.unwrap_bind(
"simple case default",
EvalLitExpr { lit: Value::Null }.bind::<{ STRICT }>(vec![]),
EvalLitExpr::new(Value::Null).bind::<{ STRICT }>(vec![]),
),
Some(def) => self.plan_value::<{ STRICT }>(def),
};
Expand All @@ -539,7 +539,7 @@ impl<'c> EvaluatorPlanner<'c> {
// If no `ELSE` clause is specified, use implicit `ELSE NULL` (see section 6.9, pg 142 of SQL-92 spec)
None => self.unwrap_bind(
"searched case default",
EvalLitExpr { lit: Value::Null }.bind::<{ STRICT }>(vec![]),
EvalLitExpr::new(Value::Null).bind::<{ STRICT }>(vec![]),
),
Some(def) => self.plan_value::<{ STRICT }>(def.as_ref()),
};
Expand Down
12 changes: 9 additions & 3 deletions partiql-value/src/datum.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub trait DatumValue<D>: Clone + Datum<D>
where
D: Datum<D>,
{
fn lower(self) -> DatumLowerResult<D>;
fn into_lower(self) -> DatumLowerResult<D>;
}

pub trait DatumCategory<'a> {
Expand Down Expand Up @@ -136,6 +136,9 @@ impl<'a> DatumCategory<'a> for Value {

pub trait TupleDatum {
fn len(&self) -> usize;
fn is_empty(&self) -> bool {
self.len() == 0
}
}

pub trait RefTupleView<'a, DV: DatumValue<DV>>: TupleDatum {
Expand Down Expand Up @@ -190,6 +193,9 @@ impl OwnedTupleView<Value> for DatumTupleOwned {
pub trait SequenceDatum {
fn is_ordered(&self) -> bool;
fn len(&self) -> usize;
fn is_empty(&self) -> bool {
self.len() == 0
}
}

pub trait RefSequenceView<'a, DV: DatumValue<DV>>: SequenceDatum {
Expand Down Expand Up @@ -223,7 +229,7 @@ impl<'a> RefSequenceView<'a, Value> for DatumSeqRef<'a> {
fn get_val(&self, k: i64) -> Option<Cow<'a, Value>> {
match self {
DatumSeqRef::List(l) => List::get(l, k).map(Cow::Borrowed),
DatumSeqRef::Bag(b) => {
DatumSeqRef::Bag(_) => {
todo!("TODO [EMBDOC]: Bag::get")
}
DatumSeqRef::Dynamic(boxed) => boxed.get_val(k),
Expand All @@ -249,7 +255,7 @@ impl OwnedSequenceView<Value> for DatumSeqOwned {
fn take_val(self, k: i64) -> Option<Value> {
match self {
DatumSeqOwned::List(l) => l.take_val(k),
DatumSeqOwned::Bag(b) => todo!("TODO [EMBDOC]: Bag::get"),
DatumSeqOwned::Bag(_) => todo!("TODO [EMBDOC]: Bag::get"),
DatumSeqOwned::Dynamic(boxed) => boxed.take_val_boxed(k),
}
}
Expand Down
Loading

0 comments on commit 8921cc7

Please sign in to comment.