start of work to match cql2 against json #55

bitner · 2024-12-12T21:26:43Z

WORK IN PROGRESS
TODO:

bitner · 2024-12-12T21:31:32Z

So, trying to set this up so that the reduce function can actually be used for any CQL2 expression to collapse things that don't need to go through to the base data. So say you had a CQL2 query like foo = 2 +7, you could use reduce to convert that to foo = 9 . To do that, I'll need to make the json input an option, if it can look up a property in the json, it will replace it with the value in that json. The matches function just looks to see if running reduce has boiled the entire expression down into an Expr::Bool and if so returns that value, if it couldn't reduce it all the way it returns an error.

kylebarron · 2024-12-13T05:04:10Z

Cargo.toml

@@ -24,9 +24,11 @@ keywords = ["cql2"]

 [dependencies]
 boon = "0.6.0"
+derive_is_enum_variant = "0.1.1"


FWIW I use https://crates.io/crates/enum-as-inner for this functionality. It looks to be much more popular and more recently maintained.

kylebarron

I don't really know CQL so I didn't look too closely at the body of reduce (yet)

kylebarron · 2024-12-13T05:05:11Z

src/expr.rs

@@ -35,7 +37,222 @@ pub enum Expr {
    Geometry(Geometry),
 }

+impl From<Value> for Expr {
+    fn from(v: Value) -> Expr {
+        let e: Expr = serde_json::from_value(v).unwrap();


Nit: If there's an unwrap in there, you usually should be implementing TryFrom instead of From.

kylebarron · 2024-12-13T05:05:31Z

src/expr.rs

+}
+
+
+impl TryInto<f64> for Expr {


You should implement TryFrom instead of TryInto: https://doc.rust-lang.org/std/convert/trait.TryInto.html

cli/src/lib.rs

src/expr.rs

gadomski · 2024-12-16T14:16:01Z

src/expr.rs

+    /// );
+    ///
+    /// let mut expr: Expr = serde_json::from_value(expr_json).unwrap();
+    /// println!("Initial {:?}", expr);


I know we're WIP testing, but I find that println debugging is quicker+better when it's in library tests, not in doctests.

Also when it's a library test and not a doctest you can just use dbg!() instead of having to use string formatting.

Heh. I am well known for copious use of print statements when in WIP mode. Will definitely clear when marking for merge.

src/expr.rs

gadomski · 2024-12-16T14:24:49Z

src/expr.rs

+    ///
+    ///
+    /// ```
+    pub fn reduce(&mut self, j: &Value) {


I think this might be more ergonomic as a consume-then-return:

Suggested change

pub fn reduce(&mut self, j: &Value) {

pub fn reduce(self, j: &Value) -> Expr {

That way you can just return directly from those match branches below rather than assigning to *self.

gadomski · 2024-12-16T14:29:44Z

src/expr.rs

+                    let b: Result<bool, _> = arg.as_ref().clone().try_into();
+                    match b {
+                        Ok(true) => anytrue = true,
+                        Ok(false) => {
+                            alltrue = false;
+                        }
+                        _ => {
+                            alltrue = false;
+                            allbool = false;
+                        }
+                    }


Not sure about the .as_ref().clone() removal, but I'd guess there'd be a way to avoid that? I'm working in the browser right now so don't have IDE powers to make me less dumb 😄:

Suggested change

let b: Result<bool, _> = arg.as_ref().clone().try_into();

match b {

Ok(true) => anytrue = true,

Ok(false) => {

alltrue = false;

}

_ => {

alltrue = false;

allbool = false;

}

}

if let Ok(bool) = arg.try_into() {

if bool {

anytrue = true;

} else {

alltrue = false;

}

} else {

alltrue = false;

allbool = false;

// Can you break here?

}

Can't break there as the main thing that loop is doing is to run reduce against each arg. We are just doing the bool check as an addition so we only have to loop through the args once.

gadomski · 2024-12-16T14:30:51Z

src/expr.rs

+        let mut e = self.clone();
+        e.reduce(j);
+        match e {
+            Expr::Bool(v) => Ok(v),
+            _ => Err(()),
+        }


How often will we re-use matches? Can we make this consuming (taking self) and let the caller do a clone themselves? Cloning inside a simple method like this feels a bit off.

Co-authored-by: Pete Gadomski <[email protected]>

start of work to match cql2 against json

cef5164

bitner requested review from gadomski and kylebarron December 12, 2024 21:26

add boolean ops, add reduce option to cli

8ac73fa

kylebarron reviewed Dec 13, 2024

View reviewed changes

gadomski requested changes Dec 16, 2024

View reviewed changes

bitner and others added 6 commits December 16, 2024 09:19

Update src/expr.rs

93d7940

Co-authored-by: Pete Gadomski <[email protected]>

Update src/expr.rs

fa337c4

Co-authored-by: Pete Gadomski <[email protected]>

add support for temporal and geospatial operators, add reduce tests

bf6c285

mend

f9a6c22

mend

7eff282

mend

56b56e6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

start of work to match cql2 against json #55

start of work to match cql2 against json #55

bitner commented Dec 12, 2024 •

edited

Loading

bitner commented Dec 12, 2024

kylebarron Dec 13, 2024

kylebarron left a comment

kylebarron Dec 13, 2024

kylebarron Dec 13, 2024

gadomski Dec 16, 2024

kylebarron Dec 16, 2024

bitner Dec 16, 2024

gadomski Dec 16, 2024

gadomski Dec 16, 2024

bitner Dec 16, 2024

gadomski Dec 16, 2024

	pub fn reduce(&mut self, j: &Value) {
	pub fn reduce(self, j: &Value) -> Expr {

start of work to match cql2 against json #55

Are you sure you want to change the base?

start of work to match cql2 against json #55

Conversation

bitner commented Dec 12, 2024 • edited Loading

bitner commented Dec 12, 2024

Choose a reason for hiding this comment

kylebarron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitner commented Dec 12, 2024 •

edited

Loading