refactor: Change terminology from query to request #1054

shahzadlone · 2023-01-24T17:37:23Z

RELEVANT ISSUE(S)

Resolves #455

DESCRIPTION

This PR changes existing terminology to comply with the following terminology definitions:

DefraDB Terminology:

(1) Request is the top-level term that encapsulates all the operations (query and mutation ops).
(2) Query Request or Request Query are same as (1) but only for situations where we need to use the word "Query" for some reason (say for external use for example in cli package), so the Request descriptor clarifies the type of "Query" this is.
(3) Query Operation implies that the request is a "read-only request", but note that if we loosely say Query Request that does not mean a read-only operation according to (2).
(4) Query loosely the same as (3), BUT STRONGLY AVOID USING ON IT'S OWN.
(5) Mutation Operation implies that the request is a "write request".
(6) Mutation Request implies that the request's operation is of type mutation. Note: (6) and (5) can be used interchangeably but (2) and (3) can not.
(7) Mutation is loosely the same as (5) and (6) but avoid using this term.

With the above definition, we can use other descriptors to specify the type of request it is, for example: "Explain request" or "Subscription Request". But the only gotcha / confusing rule we need to worry about is that Query Request is not the same as Query Operation. The use of the word Query with another descriptor assumes (4), for example: Query Plan is a plan that is made from a read-only query.

Throughout the codebase (excluding 3rd party stuff, eg. ipfs), we should avoid the use of (2), (4), and (7) to avoid confusion. The only exception to (4) I can see is when we say "Defra Query Language" (because this is not referring to the query operation).

codecov · 2023-01-24T17:42:28Z

Codecov Report

Merging #1054 (5856242) into develop (8dd7f84) will increase coverage by 0.07%.
The diff coverage is 71.04%.

@@             Coverage Diff             @@
##           develop    #1054      +/-   ##
===========================================
+ Coverage    68.03%   68.10%   +0.07%     
===========================================
  Files          171      172       +1     
  Lines        16250    16250              
===========================================
+ Hits         11055    11067      +12     
+ Misses        4261     4251      -10     
+ Partials       934      932       -2

Impacted Files	Coverage Δ
api/http/errors.go	`100.00% <ø> (ø)`
cli/root.go	`48.23% <ø> (ø)`
client/descriptions.go	`84.61% <ø> (ø)`
client/document.go	`64.25% <ø> (ø)`
client/request/select.go	`100.00% <ø> (ø)`
client/request/subscription.go	`100.00% <ø> (ø)`
db/collection.go	`65.87% <ø> (ø)`
db/collection_delete.go	`29.41% <0.00%> (ø)`
db/db.go	`72.47% <ø> (ø)`
db/fetcher/versioned.go	`56.11% <ø> (ø)`
... and 33 more

- Rename the folder. - Update all import modules that need updating due to the rename. - Update a comment that needed updating. - Update the `request/` package name.

- Rename `planner/query.go` file to `planner/request.go`. - Rename the type `planner.Query` to `planner.Request`. - Update a comment about `ExecQuery` (which will soon be changed).

NOTE: Some instances are left as `query` as this is more user facing.

Change to `PostSubscriptionRequests` & `TransactionalRequests`

AndrewSisley

LGTM - thanks a bunch for sorting this out Shahzad, would suggest merging this really aggressively to mimimize any conflicts (including for those working on other branches that will be affected by this merge)

orpheuslummis · 2023-01-24T19:19:40Z

trivia: etymologically, request and query come from the same quaerō (latin) which essentially means to want

orpheuslummis

LGTM but there are a lot of repetitive lines that I went through quickly

I saw a non-gofmt-ed notice, that would be good to fix it before it's merged.

tests/integration/utils.go

orpheuslummis · 2023-01-24T19:36:12Z

I saw a non-gofmt-ed notice, that would be good to fix it before it's merged.

I think you fixed it already in a later commit

shahzadlone · 2023-01-24T22:14:42Z

request/graphql/schema/schema.go

@@ -19,5 +19,5 @@ import (
 )

 var (
-	log = logging.MustNewLogger("defra.query.schema")
+	log = logging.MustNewLogger("defra.request.schema")


question: Should this be defra.request.graphql.schema instead @jsimnz ?

suggestion: This is a very large PR that could potentially disrupt other developers. Keeping these changes in isolation for the sake of very minor stuff like this is not going to help things and will increase the likelihood of it costing any of us time. The question you asked is not really specific to this PR (issue existed before), and can easily be sorted out after the many lines that this PR changes have been merged.

Understand your concern and agree with not delaying this PR, John is reviewing it as we speak so thought will ask a question I had before I forget. This shouldn't really effect the speed of this PR merging.

Just noting that this PR moves several files/directories that I am actively working with (including new files which definitely will not be auto-magically handled by git).

Additionally, IMO a rename PR on an item that has already been agreed upon (and reviewed in draft format) really doesnt need 3 approvals, and had I known that the majority of the dev team was going to review it prior to merge I would not have felt that adding my review to that list would have been worth the time that I spent on this.

It isn't that it needs 3 approvals, but more that if someone is taking a look at it then why not wait for them to finish the review?

jsimnz

Approving for now as my suggestions aren't necessarily for blocking. Some are just for seeding discussion as well as indexing of thoughts.

Two primary concerns (again non blocking) - There is some minor confusion regarding what a "request" is, as "request" is also used to refer to "query operation" as well as the top level GQL request.

Second, one thing we didn't reach too much consensus on regarding this change is the goals highlighted in #864 of seperating GQL from the Defra Query layer. This PR changes that conversation a bit to refer to all Defra Query layer as Requests, when in reality this change was caused by the GQL confusion specifically.

Non blocking, but wanted to see what ppl think
cc: @AndrewSisley since he started #864

db/collection_delete.go

db/collection_update.go

jsimnz · 2023-01-24T22:39:43Z

planner/plan.go

-// Query is an external hook into the planNode
+// RequestPlan is an external hook into the planNode
 // system. It allows outside packages to
-// execute and manage a query plan graph directly.
+// execute and manage a request plan graph directly.
 // Instead of using one of the available functions
-// like ExecQuery(...).
+// like ExecRequest(...).
 // Currently, this is used by the collection.Update
 // system.
-type Query planNode
+type RequestPlan planNode


suggestion: Can likely be just Request, since the package name is planner, so written it would be planner.Request which more succinct then planner.RequestPlan.

Likely why it was just Query instead of QueryPlan before.

This file was previously called query.go, by your suggestion this file would be called request.go? Would like to avoid using request.go file for plan stuff.

I would also suggest that the planNode is more a plan than a request at this stage (as the request will be converted into a plan by the time this type is used IRRC).

IMO planner.Request is still ambiguous, can be easily confused as a request to the planner for example. As planner is something that "creates a plan", and plan is the actual graph of steps.

jsimnz · 2023-01-25T13:28:35Z

request/doc.go

-package query
+package request


I'm somewhat hesitant to rename this package since the terminology confusion/replacement imo is scoped to GQL. Fundementally this is still the "DefraDB Query Language" - of which we have GQL Requests, which contains a GQL Query Operation.

This somewhat goes along with a discussion previously regarding the seperation of GQL from the Query interface #864.

I have linked this as a thing to discuss in #864, as this is non-blocking we can discuss and change it back if needed.

shahzadlone · 2023-01-26T09:42:21Z

Approving for now as my suggestions aren't necessarily for blocking. Some are just for seeding discussion as well as indexing of thoughts.

Thanks appreciate the planting of seeds that will grow into fruitful discussions.

Two primary concerns (again non blocking) - There is some minor confusion regarding what a "request" is, as "request" is also used to refer to "query operation" as well as the top level GQL request.

This is a good question, let me see if I can clarify the definitions according to this PR:

According to GQL:

Request
 | --> Query Operation
 | --> | --> Selection Set
 | --> Mutation Operation
 | --> | --> Selection Set

DefraDB Terminology:
(1) Request is the top-level term that encapsulates all the operations (query and mutation ops).
(2) Query Request or Request Query are same as (1) but only for situations where we need to use the word "Query" for some reason (say for external use for example in cli package), so the Request descriptor clarifies the type of "Query" this is.
(3) Query Operation implies that the request is a "read-only request", but note that if we loosely say Query Request that does not mean a read-only operation according to (2).
(4) Query loosely the same as (3), BUT STRONGLY AVOID USING ON IT'S OWN.
(5) Mutation Operation implies that the request is a "write request".
(6) Mutation Request implies that the request's operation is of type mutation. Note: (6) and (5) can be used interchangeably but (2) and (3) can not.
(7) Mutation is loosely the same as (5) and (6) but avoid using this term.

With the above definition, we can use other descriptors to specify the type of request it is, for example: "Explain request" or "Subscription Request". But the only gotcha / confusing rule we need to worry about is that Query Request is not the same as Query Operation. The use of the word Query with another descriptor assumes (4), for example Query Plan is a plan that is made from a read-only query.

IMO in the code base (excluding 3rd party stuff, eg. ipfs), we should avoid the use of (2), (4), and (7) to avoid confusion. The only exception to (4) I can see is when we say "Defra Query Language", but to be fair even the Q in GQL stands for query haha.

One other benefit of using these definitions is that, while they work "nicely" (maybe not perfectly) with GQL it still gives us the control to keep the terminology consistent across other query languages as we add support for them.

Second, one thing we didn't reach too much consensus on regarding this change is the goals highlighted in #864 of seperating GQL from the Defra Query layer. This PR changes that conversation a bit to refer to all Defra Query layer as Requests, when in reality this change was caused by the GQL confusion specifically.

Non blocking, but wanted to see what ppl think cc: @AndrewSisley since he started #864

This PR should comply the definition rules I defined above, if you notice any rules were broken we can change that easily in the later PR, and hope this will help keep things in harmony for #864 as well, the more reason to not depend on terms only dependent on a specific query language.

- Resolves #455 - DESCRIPTION: This PR changes existing terminology to comply with the following terminology definitions: DefraDB Terminology: * (1) `Request` is the top-level term that encapsulates all the operations (`query` and `mutation` ops). * (2) `Query Request` or `Request Query` are same as (1) but only for situations where we need to use the word "Query" for some reason (say for external use for example in `cli` package), so the `Request` descriptor clarifies the type of "Query" this is. * (3) `Query Operation` implies that the request is a "read-only request", but note that if we loosely say `Query Request` that does not mean a read-only operation according to (2). * (4) `Query` loosely the same as (3), BUT STRONGLY AVOID USING ON IT'S OWN. * (5) `Mutation Operation` implies that the request is a "write request". * (6) `Mutation Request` implies that the request's operation is of type `mutation`. Note: `(6)` and `(5)` can be used interchangeably but `(2)` and `(3)` can not. * (7) `Mutation` is loosely the same as (5) and (6) but avoid using this term. With the above definition, we can use other descriptors to specify the type of request it is, for example: "Explain request" or "Subscription Request". But the only gotcha / confusing rule we need to worry about is that `Query Request` is not the same as `Query Operation`. The use of the word `Query` with another descriptor assumes (4), for example: `Query Plan` is a plan that is made from a read-only query. Throughout the codebase (excluding 3rd party stuff, eg. ipfs), we should avoid the use of (2), (4), and (7) to avoid confusion. The only exception to (4) I can see is when we say "Defra Query Language" (because this is not referring to the query operation).

- Resolves sourcenetwork#455 - DESCRIPTION: This PR changes existing terminology to comply with the following terminology definitions: DefraDB Terminology: * (1) `Request` is the top-level term that encapsulates all the operations (`query` and `mutation` ops). * (2) `Query Request` or `Request Query` are same as (1) but only for situations where we need to use the word "Query" for some reason (say for external use for example in `cli` package), so the `Request` descriptor clarifies the type of "Query" this is. * (3) `Query Operation` implies that the request is a "read-only request", but note that if we loosely say `Query Request` that does not mean a read-only operation according to (2). * (4) `Query` loosely the same as (3), BUT STRONGLY AVOID USING ON IT'S OWN. * (5) `Mutation Operation` implies that the request is a "write request". * (6) `Mutation Request` implies that the request's operation is of type `mutation`. Note: `(6)` and `(5)` can be used interchangeably but `(2)` and `(3)` can not. * (7) `Mutation` is loosely the same as (5) and (6) but avoid using this term. With the above definition, we can use other descriptors to specify the type of request it is, for example: "Explain request" or "Subscription Request". But the only gotcha / confusing rule we need to worry about is that `Query Request` is not the same as `Query Operation`. The use of the word `Query` with another descriptor assumes (4), for example: `Query Plan` is a plan that is made from a read-only query. Throughout the codebase (excluding 3rd party stuff, eg. ipfs), we should avoid the use of (2), (4), and (7) to avoid confusion. The only exception to (4) I can see is when we say "Defra Query Language" (because this is not referring to the query operation).

shahzadlone requested a review from a team January 24, 2023 17:41

shahzadlone self-assigned this Jan 24, 2023

shahzadlone added documentation Improvements or additions to documentation refactor This issue specific to or requires *notable* refactoring of existing codebases and components code quality Related to improving code quality labels Jan 24, 2023

shahzadlone added this to the DefraDB v0.5 milestone Jan 24, 2023

shahzadlone mentioned this pull request Jan 24, 2023

refactor: Rename all Query terminology to Request #497

Closed

3 tasks

shahzadlone added 22 commits January 24, 2023 12:49

PR: Rename the query folder to request.

03fbddb

- Rename the folder. - Update all import modules that need updating due to the rename. - Update a comment that needed updating. - Update the `request/` package name.

PR: Split the mixed query & request parsing logic.

2a11c3e

PR: Change ParseQuery to ParseRequest.

99af856

PR: Fix error handling & comment for ParseRequest.

ba82859

PR: Rename planner/query.go & planner.Query.

974db30

- Rename `planner/query.go` file to `planner/request.go`. - Rename the type `planner.Query` to `planner.Request`. - Update a comment about `ExecQuery` (which will soon be changed).

PR: Fix terminology in planner, with some errors.

11d45be

PR: Fix a log I missed in schema package.

f7c4ffe

PR: Rename db/query.go file to db/request.go.

5e6fc2c

PR: Rename ExecQuery & ExecTransactionalQuery.

925bbbc

PR: Rename local vars in db/request.go.

ff9fd93

PR: Rename client.QueryResult.

30f9f86

PR: Update terminology under the db package.

6f57abe

PR: Fix terminology within the api package.

2bcd2ba

PR: Fix terminology in client package.

a07810c

PR: Rename cli/query.go to cli/request.go.

ac2f84c

PR: Fix terminology in the cli package.

5dc0b85

NOTE: Some instances are left as `query` as this is more user facing.

PR: Test - Fix terminology for subsciption tests.

bc4dcb2

PR: Test - Fix terminology for transactional tests

a14585e

PR: Test - Rename QueryTestCase.

132ed10

PR: Test - Fix 2 members of RequestTestCase.

9b530f2

Change to `PostSubscriptionRequests` & `TransactionalRequests`

PR: Test - Rename Query to Request.

f150b7a

PR: Rename ExecuteQueryTestCase.

b59bdaf

PR: More changes of QueryTestCase to Request...

e5c70aa

shahzadlone force-pushed the lone/refactor/change-query-term-to-request branch from 87a8a2c to e5c70aa Compare January 24, 2023 17:52

PR: Make linter happy make lint:fix

f4af6b7

This comment was marked as spam.

Sign in to view

shahzadlone added the action/no-benchmark Skips the action that runs the benchmark. label Jan 24, 2023

AndrewSisley approved these changes Jan 24, 2023

View reviewed changes

orpheuslummis approved these changes Jan 24, 2023

View reviewed changes

tests/integration/utils.go Outdated Show resolved Hide resolved

shahzadlone added 2 commits January 24, 2023 16:48

PR: Some terminology stuff that was missed.

5cbe3b1

PR: Adhere to PR suggestion about typo.

e55a45b

shahzadlone force-pushed the lone/refactor/change-query-term-to-request branch from d91f4cd to e55a45b Compare January 24, 2023 21:49

shahzadlone commented Jan 24, 2023

View reviewed changes

jsimnz approved these changes Jan 25, 2023

View reviewed changes

shahzadlone mentioned this pull request Jan 26, 2023

[Epic] Decouple defra from gql #864

Closed

PR: Adhere to code review todo makeSelectionPlan

5856242

shahzadlone merged commit 93ea57a into develop Jan 26, 2023

shahzadlone deleted the lone/refactor/change-query-term-to-request branch January 26, 2023 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Change terminology from query to request #1054

refactor: Change terminology from query to request #1054

shahzadlone commented Jan 24, 2023 •

edited

Loading

codecov bot commented Jan 24, 2023 •

edited

Loading

This comment was marked as spam.

AndrewSisley left a comment

orpheuslummis commented Jan 24, 2023

orpheuslummis left a comment

orpheuslummis commented Jan 24, 2023

shahzadlone Jan 24, 2023

AndrewSisley Jan 24, 2023

shahzadlone Jan 24, 2023

AndrewSisley Jan 24, 2023

shahzadlone Jan 24, 2023

jsimnz left a comment

jsimnz Jan 24, 2023

shahzadlone Jan 26, 2023

jsimnz Jan 25, 2023

shahzadlone Jan 26, 2023

shahzadlone commented Jan 26, 2023 •

edited

Loading

refactor: Change terminology from query to request #1054

refactor: Change terminology from query to request #1054

Conversation

shahzadlone commented Jan 24, 2023 • edited Loading

RELEVANT ISSUE(S)

DESCRIPTION

codecov bot commented Jan 24, 2023 • edited Loading

Codecov Report

This comment was marked as spam.

AndrewSisley left a comment

Choose a reason for hiding this comment

orpheuslummis commented Jan 24, 2023

orpheuslummis left a comment

Choose a reason for hiding this comment

orpheuslummis commented Jan 24, 2023

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

jsimnz 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

shahzadlone commented Jan 26, 2023 • edited Loading

shahzadlone commented Jan 24, 2023 •

edited

Loading

codecov bot commented Jan 24, 2023 •

edited

Loading

shahzadlone commented Jan 26, 2023 •

edited

Loading