refactor: rename 'expression' for clarity#26
Conversation
8f5350c to
9023ebf
Compare
| * { date: { relative: { offset: 0, unit: "month", boundary: "start" } } } | ||
| */ | ||
| export type DateOperand = | ||
| export type DateExpression = |
There was a problem hiding this comment.
let's rename the parent folder to be expression
There was a problem hiding this comment.
Ah yes, I was leaving the folder rename for later to minimize diffs
| { model: string, expression: Expression } | ||
| | // unary expression | ||
| { operator: UnaryOperator, operand: Operand } | ||
| { operator: UnaryOperator, operand: Expression|Expression[] } |
There was a problem hiding this comment.
should we rename operand to expression?
There was a problem hiding this comment.
No, I think it's ok
| export type Audience = { | ||
| schema_version: Version, | ||
| audience: Expression | ||
| audience: Condition |
There was a problem hiding this comment.
This feels a bit semantically awkward in my brain. I wonder if we should rename the attribute something like filter: Condition?
There was a problem hiding this comment.
This doesn't bother me. The root audience key is the entry point into defining the criteria for an audience, which is a Condition
| "expressions": [ | ||
| "conditions": [ |
There was a problem hiding this comment.
Should this be expression still? In my mind, condition is a filter operation. Likely only relevant to aggregate operations. I would say:
- Expression == thing the operator is acting on
- Condition == expression that determines which things are operated on (aka, filters the input)
There was a problem hiding this comment.
My vote is to leave it as is. In my mind logical condition can only contain conditions and Condition == expression that returns boolean
There was a problem hiding this comment.
No, Condition applies everywhere, not just aggregate operations. The root expression for an audience evaluates to a boolean to determine membership, e.g.
{
"operator": "and",
"conditions": [
{ .. condition 1 .. },
{ .. condition 2 .. },
]
}
is itself a Condition
There was a problem hiding this comment.
The root expression for an audience evaluates to a boolean to determine membership
I didn't notice this was directly on the audience, but I agree the top level should be a condition. Not a fan of the current syntax, honestly - I would do:
"audience" : {
"condition": { some bool expression },
}That way the top level isn't forced to follow the expression syntax, which is more flexible for the future.
I think the other expressions should only use condition as a filter though - so any nested and/or would NOT use condition since they aren't checking a condition, just evaluating an expression. Condition implies check to me, but there are only two checks currently, on aggregates and at the base query.
Instructions
developmentSummary
ExpressiontoConditionOperandtoExpressionExpressionsthat didn't cleanly fit. For example DateExpression, "Represents an expression that evaluates to a date or date-based condition". Fairly certain these are not being usedgroup_bytogroup_by_modelper prior discussionsTesting Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)