[RFC] Filtering over nested arrays of scalars

[daniel-chambers]

Note: This PR is currently stacked on #187 and will be rebased once that PR has merged. Rebased on main.

RFC Rendered

This PR also makes some minor quality of life improvements:

• Adds some useful targets to the justfile
• Widens the width of content in the ndc-spec documentation website by 150px to help give a bit more space to accommodate the capabilities tables

• RFC
• Specification updates
  • Changelog
  • Specification pages
  • Tutorial pages
  • reference/types.md
• Implement your feature in the reference connector
• Generate test cases in ndc-test if appropriate
• Does your feature add a new capability?
  • Update specification/capabilities.md in the specification
  • Check the capability in ndc-test

[hallettj]

Looks great! I appreciate the new comparison target idea since that makes disambiguating easier for the connector. I agree that this should only apply to arrays of scalars.

[daniel-chambers]

There is a problem with this approach. It doesn't allow use of logical operators beyond the new ArrayComparison::ExistsInNestedArray boundary. So, for example, if the following data existed:

[
  Customer {
    nested_numbers: [ [2,1], [1,0] ]
  },
  Customer {
    nested_numbers: [ [2,3], [1,0] ]
  }
]  

and I wanted to ask the following question:

give me all customers where the inner array inside nested_numbers contains 1 and also contains 2

query {
  Customer(where: { nested_numbers: { inner: { _and: [ { _eq: 1 }, { _eq: 2 } ] } } }) {
    id
  }
}

I couldn't because there's no way to nest logical operators inside a ArrayComparison::ExistsInNestedArray.

collection: customers
query:
  fields:
    id:
      type: column
      column: id
  predicate:
    type: array_comparison
    column:
      type: column
      name: nested_numbers
      path: []
    comparison:
      type: exists_in_nested_array
      nested_comparison:
        type: and # This does not exist
        comparisons:
          - type: exists_binary
            operator: eq
            value:
              type: literal
              value: 1
          - type: exists_binary
            operator: eq
            value:
              type: literal
              value: 2

arguments: {}
collection_relationships: {}

Perhaps a simple solution to this, that doesn't require another level of "And", "Or", "Not" inside ArrayComparison, is to make ArrayComparison::ExistsInNestedArray take an array of nested comparisons which are implicitly "and-ed" together.

enum ArrayComparison {
    ...
    ExistsInNestedArray {
        nested_comparisons: Vec<ArrayComparison>
    }
}

This would allow us to do logical conjunction (ie "and") within the same exists scope. For disjunction, my understanding is that we can always lift the logical "or" to the top of the expression. Here's me asking ChatGPT about it: https://chatgpt.com/share/66fa38a6-c7a4-8000-a290-6af0aaa258a5 (its answer about "Existential Quantification and Conjunction (∃ and ∧)" is revealing)

[paf31]

For disjunction, my understanding is that we can always lift the logical "or" to the top of the expression. Here's me asking ChatGPT about it: https://chatgpt.com/share/66fa38a6-c7a4-8000-a290-6af0aaa258a5 (its answer about "Existential Quantification and Conjunction (∃ and ∧)" is revealing)

Only if you want to perform DNF translation or something else whenever an OR moves past an AND.

[hallettj]

I had a thought - we had said that it would be helpful if Expression were partially-applied instead of assuming that every expression evaluates in the context of a record. Could we get to a similar place by adding a variant to ComparisonTarget like ComparisonTarget::ContextuallyRelevantValue? Then instead of defining a parallel set of expressions for applying to arrays we could basically duplicate the Exists variant.

pub enum Expression {
    // ...
    ExistsInArray {
        column: ComparisonTarget,
        predicate: Option<Box<Expression>>,
    },
}

pub enum ComparisonTarget {
    // ...
    ContextuallyRelevantValue, // unit variant
}

In that formulation given the GraphQL query,

query {
  Customer(where: { nested_numbers: { inner: { _and: [ { _eq: 1 }, { _eq: 2 } ] } } }) {
    id
  }
}

I'm imagining an NDC query like this,

collection: customers
query:
  fields:
    id:
      type: column
      column: id
  predicate:
    type: exists_in_array
    column:
      type: column
      name: nested_numbers
      path: []
    predicate:
      type: exists_in_array
      column: { type: "contextually_relevant_value" }
      predicate:
        type: and
        expressions:
          - type: binary_comparison_operator
            column: { type: "contextually_relevant_value" }
            operator: eq
            value:
              type: literal
              value: 1
          - type: binary_comparison_operator
            column: { type: "contextually_relevant_value" }
            operator: eq
            value:
              type: literal
              value: 2

arguments: {}
collection_relationships: {}

But @daniel-chambers I think I disagree with how the expressions apply in that query. I think that GraphQL query asks if there exists one element that is equal to 1 and also equal to 2 so I expect that query wouldn't return any rows. To filter where the inner array contains possibly-different elements where one is equal to 1 and one is equal to 2 I think the and operator should be moved higher:

query {
  Customer(where: { nested_numbers: { _and: [{ inner: { _eq: 1 } }, { inner: { _eq: 2 } }] } }) {
    id
  }
}

But either way I see your point that we need a change to allow boolean conjunctions.

So my idea might lead to some possible nonsensical expressions. But we can avoid problems by not producing such expressions. I think it would be better to change the Expression type to use that partial-application idea. But maybe extending ComparisonTarget is easier.

[paf31]

Along the same lines as contextually_relevant_value, we could maybe solve this by adding a new variant to ExistsInCollection:

pub enum ExistsInCollection {
    Related {..},
    Unrelated {..},
    NestedCollection {..},
    NestedScalarCollection {
        column_name: FieldName,
        arguments: BTreeMap<ArgumentName, Argument>,
        field_path: Vec<FieldName>,
    }

NestedScalarCollection explodes an array of scalars into a nested collection, with a single column whose name can be static and chosen by us, similar to __value for functions. The caller can then use the usual expressions to filter by rows of this nested collection.

[hallettj]

@paf31 Does ExistsInCollection::NestedScalarCollection handle cases of arrays of arrays? Either an array of arrays of scalars, or an array of arrays of records?

Edit: I guess why wouldn't those cases work? Sounds good.

[daniel-chambers]

I think that GraphQL query asks if there exists one element that is equal to 1 and also equal to 2 so I expect that query wouldn't return any rows.

@hallettj It's not asking if there exists one element, it's there exists any element in the inner array. Remember this is a doubly-nested array in the example, so "inner" represents an array of ints. So its the same inner array that must contain both 1 and 2. Here's an example:

[ [1,4], [3,1] ] -> false
[ [4,6], [3,0] ] -> false
[ [2,6], [3,1] ] -> false
[ [2,1], [1,0] ] -> true

EDIT: Actually, I think you're right. Sorry!! I think to do what I wanted above you can't use two nested exists, you need an exists and contains (eg ∃ inner. ( CONTAINS(inner, 1) && CONTAINS(inner, 2) ), not ∃ inner. ( ∃ int. (int == 1 && int == 2 ) )).

---

I think @paf31's latest idea seems reasonable... it certainly follows an existing pattern we use for functions (a virtual column). I think I prefer it to extending ComparisonTarget with ContextuallyRelevantValue because ContextuallyRelevantValue is only really valid once you descend through Expression::ExistsInArray. Whereas hijacking ComparisonTarget::Column by introducing a virtual __value column means that the actual data structure is always valid, it's just the schema used in it (ie the column name) that might be invalid.

I'll update the RFC with the latest idea.

[daniel-chambers]

What we have lost, however, is the is_empty and contains functions over any array. We could keep Expression::ArrayComparison only keep ArrayComparison::Contains and ArrayComparison::IsEmpty to continue to support these two things.

I've updated the RFC with these ideas.

[hallettj]

Looks good!

Do we have an idea what GraphQL expressions that map to ArrayComparisons will look like? Like will contains and is_empty be reserved operator names that supersede scalar comparisons?

[daniel-chambers]

@hallettj Check out the v3-engine RFC for more information about graphql shape.

As for reserved names, how they are named in graphql is up to how you configure the naming in OpenDD. If the connector defines both and their names clash, you will need to rename one or the other.

[paf31]

Can we use subheadings here?

[daniel-chambers]

Done :)

[paf31]

Deliberate?

[daniel-chambers]

Yes, I widened the width of content in the ndc-spec documentation website by 150px to help give a bit more space to accommodate the capabilities tables

[paf31]

Looks good thanks.

Just so I understand the overlap (contains and is_empty can be expressed using exists-in-scalar-array), I assume this is meant to future-proof things for connectors which can implement one but not the other? Do we have such a connector in mind or is it speculative?

[daniel-chambers]

@paf31 ClickHouse was the connector I had in mind, because I remember @BenoitRanque saying that arbitrary existential quantification across nested arrays wasn't going to be possible.

The base branch was changed.