temporary QueryWorkflowAnalysis code #684

Kelerchian · 2024-03-07T12:45:30Z

No description provided.

rkuhn · 2024-03-07T13:20:38Z

rust/actyx/ax-aql/src/language/parser.rs

+        }
+    }
+
+    pub fn check(&'a self, query: &'a Query) -> Vec<QueryWorkflowAnalysisError> {


The query argument should be the same as self.query, right? In that case it should be removed.

rkuhn · 2024-03-07T13:21:14Z

rust/actyx/ax-aql/src/language/parser.rs

+            .workflows
+            .iter()
+            .for_each(|(_ident, workflow)| errors.extend(self.check_workflow(workflow)));
+        errors


I’d use self.query.workflows.values().flat_map(|wf| self.check_workflow(wf)).collect()

rkuhn · 2024-03-07T13:25:13Z

rust/actyx/ax-aql/src/language/parser.rs

+            }
+            Call {
+                workflow: workflow_ident,
+                cases: _,


cases need to be checked as well

rkuhn · 2024-03-07T13:26:09Z

rust/actyx/ax-aql/src/language/parser.rs

+                ..
+            } => {
+                if let Some(workflow) = self.query.workflows.get(&*workflow_ident) {
+                    self.check_workflow(workflow)


I wouldn’t do this here, since we have a reliable top-level enumeration of all workflows. This means the tracker isn’t needed.

rkuhn · 2024-03-07T13:27:36Z

rust/actyx/ax-aql/src/language/parser.rs

+}
+
+#[derive(Default)]
+pub(crate) struct WorkflowTracker<'a>(pub(crate) RefCell<BTreeSet<&'a Workflow<'a>>>);


Probably no longer needed, but still: why pub(crate)? Such internal things should always stay as local as possible, especially with a RefCell inside.

rkuhn · 2024-03-07T13:31:33Z

rust/actyx/ax-aql/src/language/parser.rs

+    pub(crate) query: &'a Query<'a>,
+    pub(crate) workflow_tracker: WorkflowTracker<'a>,


I think neither of these fields are needed: the (recursive) analysis functions can just get the outer Query and the syntax element in question as arguments. When analysing the flow inside a workflow we’ll probably need to have some more context, though, so it might be appropriate to keep this struct with the query field in any case.

I would also imagine the type checker information to be stored in this struct too because it will be used inside while checking binders

jmg-duarte

I was faced with similar vec vs iter decisions while writing the code to dive down the workflow getting the steps. Curious to learn your take on that.

jmg-duarte · 2024-03-07T17:27:51Z

rust/actyx/ax-aql/src/language/parser.rs

+    }
+
+    fn check_step(&'a self, step: &'a WorkflowStep<'a>) -> Vec<QueryWorkflowAnalysisError> {
+        use super::workflow::WorkflowStep::*;


Nitpick: Not a fan of using all the enum members. In this case is not particularly problematic but take a look into this https://youtu.be/8j_FbjiowvE?si=DOchI2_0vieFkEjg&t=103 (not the full video)

wat?? TIL thanks for the reference

rust/actyx/ax-aql/src/language/parser.rs

jmg-duarte · 2024-03-07T17:32:48Z

rust/actyx/ax-aql/src/language/parser.rs

+                    });
+                }
+
+                errors.extend(cases.iter().flat_map(|(_, steps)| self.check_steps(steps)));


Since this always runs, you can do:

let mut errors = cases.iter().flat_map(|(_, steps)| self.check_steps(steps)).collect::<Vec<_>>();

For some reason, I feel that workflow_identity error should go first before the cases error. Not that I am sure why this ordering matter at all

rust/actyx/ax-aql/src/language/parser.rs

Kelerchian · 2024-03-07T18:36:54Z

I was faced with similar vec vs iter decisions while writing the code to dive down the workflow getting the steps. Curious to learn your take on that.

My first thought was: if these methods can return an iterator rather than vec, then at the end a chain and collect is executed, then it might result in a better code. But I do not know how to do that nor I can find any references on the internet on how to do that

jmg-duarte · 2024-03-15T14:45:19Z

rust/actyx/ax-aql/src/language/type_check.rs

-                    }
-                    _ => Err(format!("{:?} cannot be accessed by {:?}", x, first)),
+            Type::Atom(type_atom) => match type_atom {
+                TypeAtom::Universal => return Ok(Type::Atom(TypeAtom::Universal)),


Shouldn't this be

return Ok(*cur_type) // or cur_type.clone() if Type doesn't implement Copy

Or even just:

return Err(anyhow!("UNIVERSAL type is not indexable"))

rust/actyx/ax-aql/src/language/type_check.rs

rkuhn · 2024-03-18T10:04:08Z

rust/actyx/ax-aql/src/language/type_check.rs

+    pub(crate) error_type: QueryTypeCheckErrorType,
+}
+
+type ExtraMessage = String;


This is usually not done in Rust: type aliases are used to write down complex types with less verbosity, otherwise newtypes like struct ExtraMessage(String) are used to create a separate type with different semantics.

With struct how can I make it so that ExtraMessage can be operated as if it is a string?

What do you want to achieve? In which way shall the new type be different from String?

I want to state that this is a string, but also an "ExtraMessage" for InvalidIndex error.
The reason is, DrillDownResult cannot be Result<Type, QueryTypeCheckErrorType::InvalidIndex> and Result<Type, String> isn't clear enough on what the string are

In that case I’d just use Result<Type, QueryTypeCheckErrorType> as the return type.

rkuhn

In general the control flow structure will need to be geared towards traversing the statements of a workflow in the same order as during evaluation, including a context that is enriched along the way. This enables checking that eventLabel @ whatever refers to a valid whatever — which may either be a workflow argument or a binding.

rkuhn · 2024-03-18T13:18:48Z

rust/actyx/ax-aql/src/language/type_check.rs

+    }
+}
+
+pub(crate) struct QueryTypeCheck<'a> {


Overall, type checking is a function, not an object, so I’d start from functions, not methods. If the argument lists become too long and repetitive, then some arguments can be passed within a single struct.

rust/actyx/ax-aql/src/language/type_check/drill_down_type.rs

jmg-duarte · 2024-03-19T16:04:17Z

rust/actyx/ax-aql/src/language/type_check/drill_down_type.rs

+                tail: vec![Index::String(String::from("some_string"))].try_into().unwrap(),
+            },
+        )
+        .is_err());


I'd add the second parameter to assert! with a short explanation of what failed

jmg-duarte · 2024-03-19T16:05:08Z

rust/actyx/ax-aql/src/language/type_check/drill_down_type.rs

+    }
+
+    #[test]
+    fn tuple() {


Nit: I'd separate all the asserts in this test into multiple tests. AFAIK you also get to take more advantage of parallelism that way

rust/actyx/ax-aql/src/language/type_check/mod.rs

jmg-duarte · 2024-03-19T16:08:41Z

rust/actyx/ax-aql/src/language/type_check/mod.rs

I think this file is lacking some docs, they should help you pick the file back up in the future too as well as any potential contributors.

rust/actyx/ax-aql/src/language/types.rs

…-machine

jmg-duarte · 2024-03-22T10:39:53Z

rust/actyx/ax-aql/src/language/types.rs

+            (Type::Record(a), Type::Record(b)) => {
+                let a = a.iter().collect::<BTreeSet<_>>();
+                let b = b.iter().collect::<BTreeSet<_>>();
+                let intersection = BTreeSet::intersection(&a, &b).cloned().collect::<BTreeSet<_>>();


Consider the following records:

type A = { x: number, y: string } type B = { y: string }

As far as your code goes, this makes it so that A supertypeof B which I agree with, however, what if we have the following:

type A = { x: number, y: { y_x: string, y_z: string } } type B = { x: number, y: { y_x: string } }

Logic follows that A supertypeof B is still true, however I don't think this piece of code handles this case.

I haven’t yet read the code, but @jmg-duarte your comment above is incorrect: a subtype can be used in place of its supertype (Liskov substitution principle), which means that it must have at least the properties of the supertype. A is a subtype of B, in both cases.

really nice catch. thanks for this

https://www.cs.cornell.edu/courses/cs6110/2018sp/lectures/lec23.pdf

I haven’t yet read the code, but @jmg-duarte your comment above is incorrect: a subtype can be used in place of its supertype (Liskov substitution principle), which means that it must have at least the properties of the supertype. A is a subtype of B, in both cases.

True, I applied the logic for refinements here where NUMBER is supertype of 10.

But I think the nesting issue still applies?

The nesting issue still applies I think.

jmg-duarte · 2024-03-22T10:53:45Z

rust/actyx/ax-aql/src/language/types.rs

+    One(T),
+}
+
+fn spread<T>(vec: Vec<T>, spreader: impl Fn(&T) -> Spread<T>) -> Vec<T> {


I'm not sure I understand the point of this function, could you add docs or explain?

will do. and it will be moved down because it doesn't make sense being between struct Type and impl Type

jmg-duarte · 2024-03-22T11:04:34Z

rust/actyx/ax-aql/src/language/types.rs

+    /// Spread union tree into a set of type references without collapsing
+    fn spread_union((a, b): &(Type, Type)) -> BTreeSet<&Type> {
+        let spread = spread(vec![a, b], |x| match x {
+            Type::Union(union) => {
+                let (a, b) = union.as_ref();
+                Spread::Many(vec![a, b])
+            }
+            x => Spread::One(x),
+        });
+
+        spread.into_iter().collect()
+    }
+
+    /// Spread union tree into a set of types with collapsing
+    fn spread_union_collapsing((a, b): &(Type, Type)) -> BTreeSet<Type> {
+        let spread = spread(vec![a.clone(), b.clone()], |x| match x.clone().collapse() {
+            Type::Union(union) => {
+                let (a, b) = union.as_ref();
+                Spread::Many(vec![a.clone(), b.clone()])
+            }
+            x => Spread::One(x),
+        });
+
+        spread.into_iter().collect()
+    }
+}


I feel there must be a better way of writing this API. The double clone that ends up happening in line 337 seems wrong.

Maybe you could use a Collapse trait and implement it for the various BTreeSet, delaying the cloning until its really necessary for example. Furthermore, as we discussed, collapse probably doesn't need to consume the type, you're just checking a bunch of things and returning a new type.

while collapse doesn't consume, it still produces a new type.
this in turn forces this function to return BTreeSet and not BTreeSet<&Type>
because we cannot return references to objects created within the function (produced by collapse)

Yeah that makes sense but my point was also about how (if possible) to make a slightly better API by just having a single function instead of these two.

temporary QueryWorkflowAnalysis code

ee74b6e

Kelerchian force-pushed the ada/aql-machine branch from fd9dbfc to ee74b6e Compare March 7, 2024 12:46

rkuhn reviewed Mar 7, 2024

View reviewed changes

Kelerchian requested a review from jmg-duarte March 7, 2024 14:33

jmg-duarte reviewed Mar 7, 2024

View reviewed changes

tweaks from PR review

866c948

Kelerchian force-pushed the ada/aql-machine branch from 6c6149c to 866c948 Compare March 7, 2024 19:33

add undeclared participant check

b147069

Kelerchian force-pushed the ada/aql-machine branch from 07ecd0e to 9470962 Compare March 14, 2024 15:40

add index_tail_access_matchex

159f37b

Kelerchian force-pushed the ada/aql-machine branch from 9470962 to 159f37b Compare March 14, 2024 15:43

add SimpleExpr calculation on type check and union collapsing mechanism

832e473

Kelerchian force-pushed the ada/aql-machine branch from a9def1a to 832e473 Compare March 15, 2024 14:35

fix typo

755fa7d

jmg-duarte reviewed Mar 15, 2024

View reviewed changes

fix clippy and minimize recursion on type drill-down

2d4b569

rkuhn reviewed Mar 18, 2024

View reviewed changes

Kelerchian added 2 commits March 18, 2024 15:33

move drill_down_type deeper

08afbc3

add type collapse

3a62837

jmg-duarte reviewed Mar 19, 2024

View reviewed changes

Kelerchian added 4 commits March 20, 2024 12:59

fix union collapsing to include supertype collapsing

4282e6a

Merge branch 'rku/aql-machine' of github.com:Actyx/Actyx into ada/aql…

c628e8e

…-machine

merge aql-machine branch

1d28bca

add bottom type and fix union/intersection collapse

8a3b296

jmg-duarte reviewed Mar 22, 2024

View reviewed changes

fix hierarchy calculation of records

b160519

Kelerchian force-pushed the ada/aql-machine branch from 1c8396e to b160519 Compare March 25, 2024 08:22

		pub(crate) query: &'a Query<'a>,
		pub(crate) workflow_tracker: WorkflowTracker<'a>,

temporary QueryWorkflowAnalysis code #684

Are you sure you want to change the base?

temporary QueryWorkflowAnalysis code #684

Conversation

Kelerchian commented Mar 7, 2024

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

jmg-duarte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kelerchian Mar 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kelerchian commented Mar 7, 2024

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

rkuhn 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

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

Kelerchian Mar 7, 2024 •

edited