Skip to content

Commit

Permalink
Updated RFC with new ideas
Browse files Browse the repository at this point in the history
  • Loading branch information
daniel-chambers committed Sep 26, 2024
1 parent a25dc7f commit d306619
Show file tree
Hide file tree
Showing 3 changed files with 157 additions and 6 deletions.
68 changes: 66 additions & 2 deletions ndc-models/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,38 @@ pub struct NestedFieldCapabilities {
#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize, JsonSchema)]
#[schemars(title = "Nested Field Filter By Capabilities")]
pub struct NestedFieldFilterByCapabilities {
// Does the connector support filtering over arrays of scalars using existential quantification
pub scalar_arrays: Option<LeafCapability>
/// Does the connector support filtering over nested arrays
pub nested_arrays: Option<NestedArrayFilterByCapabilities>
}
// ANCHOR_END: NestedFieldFilterByCapabilities

// ANCHOR: NestedArrayFilterByCapabilities
#[skip_serializing_none]
#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize, JsonSchema)]
#[schemars(title = "Nested Array Filter By Capabilities")]
pub struct NestedArrayFilterByCapabilities {
/// Does the connector support filtering over nested arrays using existential quantification.
/// This must be supported for all types that can be contained in an array that have a comparison operator.
pub exists: Option<NestedFieldExistsFilterByCapabilities>,
/// Does the connector support filtering over nested arrays by checking if the array contains a value.
/// This must be supported for all types that can be contained in an array.
pub contains: Option<LeafCapability>,
/// Does the connector support filtering over nested arrays by checking if the array is empty.
/// This must be supported no matter what type is contained in the array.
pub is_empty: Option<LeafCapability>,
}
// ANCHOR_END: NestedArrayFilterByCapabilities

// ANCHOR: NestedFieldExistsFilterByCapabilities
#[skip_serializing_none]
#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize, JsonSchema)]
#[schemars(title = "Nested Array Exists Filter By Capabilities")]
pub struct NestedFieldExistsFilterByCapabilities {
/// Does the connector support filtering over nested arrays of arrays using existential quantification
pub nested: Option<LeafCapability>
}
// ANCHOR_END: NestedFieldExistsFilterByCapabilities

// ANCHOR: AggregateCapabilities
#[skip_serializing_none]
#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize, JsonSchema)]
Expand Down Expand Up @@ -817,13 +844,50 @@ pub enum Expression {
operator: ComparisonOperatorName,
value: ComparisonValue,
},
ArrayComparison {
column: ComparisonTarget,
comparison: ArrayComparison,
},
Exists {
in_collection: ExistsInCollection,
predicate: Option<Box<Expression>>,
},
}
// ANCHOR_END: Expression

// ANCHOR: ArrayComparison
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, JsonSchema)]
#[schemars(title = "Array Comparison")]
#[serde(rename_all = "snake_case")]
pub enum ArrayComparison {
/// Perform a binary comparison operation against the elements of the array.
/// The comparison is asserting that there must exist at least one element
/// in the array that the comparison succeeds for
ExistsBinary {
operator: ComparisonOperatorName,
value: ComparisonValue,
},
/// Perform a unary comparison operation against the elements of the array.
/// The comparison is asserting that there must exist at least one element
/// in the array that the comparison succeeds for
ExistsUnary {
operator: UnaryComparisonOperator
},
/// Nest a comparison through one level of a nested array, asserting that
/// there must exist at least one element in the outer array who matches
/// the comparison applied to the inner array
ExistsInNestedArray {
nested_comparison: Box<ArrayComparison>
},
/// Check if the array contains the specified value
Contains {
value: ComparisonValue,
},
/// Check is the array is empty
IsEmpty,
}
// ANCHOR_END: ArrayComparison

// ANCHOR: UnaryComparisonOperator
#[derive(
Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize, JsonSchema,
Expand Down
3 changes: 2 additions & 1 deletion ndc-reference/bin/reference/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ async fn get_capabilities() -> Json<models::CapabilitiesResponse> {
explain: None,
nested_fields: models::NestedFieldCapabilities {
filter_by: Some(models::NestedFieldFilterByCapabilities {
scalar_arrays: None,
nested_arrays: None,
}),
order_by: Some(models::LeafCapability {}),
aggregates: Some(models::LeafCapability {}),
Expand Down Expand Up @@ -1983,6 +1983,7 @@ fn eval_expression(
))?;
Ok(!rows.is_empty())
} // ANCHOR_END: eval_expression_exists
models::Expression::ArrayComparison { .. } => todo!(),
}
}
// ANCHOR_END: eval_expression
Expand Down
92 changes: 89 additions & 3 deletions rfcs/0023-filtering-over-nested-array-of-scalars.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,92 @@ collection_relationships: {}
However, this would currently be illegal since the type of the the `roles` column (array of `String`) does not match the value's type (`String`).

## Proposal
We could add another variant to Expression to represent a comparison against an array type:

```rust
pub enum Expression {
...
ArrayComparison {
column: ComparisonTarget,
comparison: ArrayComparison,
},
}
```

The `ArrayComparison` type would then capture the different types of comparisons one could do against the array:

```rust
pub enum ArrayComparison {
/// Perform a binary comparison operation against the elements of the array.
/// The comparison is asserting that there must exist at least one element
/// in the array that the comparison succeeds for
ExistsBinary {
operator: ComparisonOperatorName,
value: ComparisonValue,
},
/// Perform a unary comparison operation against the elements of the array.
/// The comparison is asserting that there must exist at least one element
/// in the array that the comparison succeeds for
ExistsUnary {
operator: UnaryComparisonOperator
},
/// Nest a comparison through one level of a nested array, asserting that
/// there must exist at least one element in the outer array who matches
/// the comparison applied to the inner array
ExistsInNestedArray {
nested_comparison: Box<ArrayComparison>
},
/// Check if the array contains the specified value
Contains {
value: ComparisonValue,
},
/// Check is the array is empty
IsEmpty,
}
```

Whether or not these new array comparisons would be supported by the connector would be declared in the capabilities:

```jsonc
{
"query": {
"aggregates": {},
"variables": {},
"nested_fields": {
"filter_by": {
// NEW!!
// Does the connector support filtering over nested arrays
"nested_arrays": {
// Does the connector support filtering over nested arrays using existential quantification.
// This must be supported for all types that can be contained in an array that have a comparison operator.
"exists": {
// Does the connector support filtering over nested arrays of arrays using existential quantification
"nested": {}
},
// Does the connector support filtering over nested arrays by checking if the array contains a value.
// This must be supported for all types that can be contained in an array.
"contains": {},
// Does the connector support filtering over nested arrays by checking if the array is empty.
// This must be supported no matter what type is contained in the array.
"isEmpty": {}
}
},
"order_by": {},
"aggregates": {}
},
"exists": {
"nested_collections": {}
}
},
"mutation": {},
"relationships": {
"relation_comparisons": {},
"order_by_aggregate": {}
}
}
```

## Alternative Proposal

We could update the definition of `ComparisonTarget::Column` to specify that if the targeted column is an array of scalars, then the comparison operator should be considered to be existentially quantified over all elements in the array. In simpler terms, at least one element in the array of scalars must match the specified comparison.

Expand Down Expand Up @@ -88,9 +174,9 @@ This behaviour for `ComparisonTarget::Column` is new, and as such would need to
}
```

## Issues
### Issues

### Implicit existential quantification
#### Implicit existential quantification

This new interpretation of the query structure is implicit, which is suboptimal as it may be non-obvious to connector authors that this is how things are supposed to work. It is better to be explicit with such things.

Expand Down Expand Up @@ -169,7 +255,7 @@ The use of `ComparisonTarget::ExistsInColumn` would be gated behind the proposed

The issue with this is that it requires more work to support, as more extensive changes are required to v3-engine so that it uses this new `ComparisonTarget`.

### How about existential quantification over arrays of nested objects?
#### How about existential quantification over arrays of nested objects?

What about if we had the following `User` and `Role` object types:

Expand Down

0 comments on commit d306619

Please sign in to comment.