Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update ndc-spec to 0.1.0-rc14 #283

Merged
merged 14 commits into from
Feb 14, 2024
Merged

update ndc-spec to 0.1.0-rc14 #283

merged 14 commits into from
Feb 14, 2024

Conversation

soupi
Copy link
Contributor

@soupi soupi commented Feb 6, 2024

What

We are updating connector to support the latest ndc-connector spec (0.1.0-rc14).
While fixing some breaking changes, such as renaming where to predicate, we also support new capabilities:

  1. Explaining mutations via the /mutation/explain endpoint
  2. Supporting filtering using in by columns and variables

How

  1. Since a mutation request may contain multiple mutation operations, the explain response will contain a field named <operation_name> SQL Mutation and a field named <operation_name> Execution Plan for each operation.
  2. To filter in column or variable, we need to compare against a subquery of the shape (select value from unnest(<column-or-variable>) as table(value)).

///
/// This function implements the [mutation/explain endpoint](https://hasura.github.io/ndc-spec/specification/explain.html)
/// from the NDC specification.
async fn mutation_explain(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new endpoint and capability - explaining mutation requests.

@@ -3,7 +3,6 @@
pub mod capabilities;
pub mod configuration;
pub mod connector;
pub mod explain;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced by a query::explain and mutation::explain modules.

@@ -50,15 +53,15 @@ pub async fn mutation<'a>(
timer.complete_with(result)
}

fn plan_mutation(
pub fn plan_mutation(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used in the explain module

@@ -108,6 +108,11 @@ pub enum From {
alias: TableAlias,
column: ColumnAlias,
},
Unnest {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used for fancy queries like in <variable> or in <column>, which are converted to a subquery of unnest.

@@ -67,116 +68,153 @@ pub fn translate_expression(
column,
operator,
value,
} => {
} if operator == "in" => {
Copy link
Contributor Author

@soupi soupi Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operators are now strings, and BinaryArrayComparisonOperator is gone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old binary array comparisons was only between a column and a vector of values. This new formulation lets us compare against scalar arrays, columns and variables. These are implemented below.

)?,
vec![],
)),
} => match predicate {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

predicates are now optional instead of being an equivalence of true when they are omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explain mutations

"type": "column",
"column": {
"type": "column",
"name": "series",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new capability: filter by "in column"

"operator": "in",
"value": {
"type": "variable",
"name": "array"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new capability: filter by "in variable"

Comment on lines -27 to +30
let timer = state.metrics.time_query_total();
let timer = state.metrics.time_mutation_total();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

Comment on lines 412 to 413
value: models::ComparisonValue,
typ: &database::ScalarType,
typ: database::Type,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could probably both be some references to avoid clones at the call sites.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks

"operator": {
"type": "equal"
},
"operator": "eq",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this one "eq" but others are "_eq"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eq refers to the built-in equals operator from ndc-spec, _eq refers to the operator defined in the metadata of some tests (in tables.json). Most tests will use eq, but some tests might have wanted to test a metadata-defined operator? I wouldn't mind changing everything to eq.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't understand this well enough to agree or disagree, so I am going to pretend I didn't ask and trust that you know what you're doing.

Copy link
Contributor Author

@soupi soupi Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ndc-spec defines two standard operators that a connector should implement - equal (eq) and in (in). Any other operator is a custom operator defined by the connector.

So for us we implement in and eq as built-in operators that do not need to be defined in the ndc-metadata, and we also allow users to define their own operators in the ndc-metadata using the comparisonOperators field. Example.

In most tests (like this one), we use the built-in eq operator. In other tests where we use the _eq operator, we have to define it like in the example I linked.

Tests that use _eq also test custom comparison operators, tests that use eq test the built-in operator.

@soupi soupi marked this pull request as ready for review February 14, 2024 09:13
@SamirTalwar
Copy link
Contributor

I think I made a big merge conflict for you. Sorry.

@soupi soupi added this pull request to the merge queue Feb 14, 2024
Merged via the queue into main with commit 553ad47 Feb 14, 2024
28 checks passed
@soupi soupi deleted the gil/ndc-spec-rc14 branch February 14, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants