[BB pr#90] KIT-76 feat(headless): add support for aq,cq,dq,lq

coveobot commented 4 years ago

Pull request :twisted_rightwards_arrows: created by @olamothe on 2020-07-28 20:34 Last updated on 2020-10-06 22:41 Original Bitbucket pull request id: 90

Participants:

@acote-coveo (reviewer)

@ThibodeauJF (reviewer)

@btaillon (reviewer)

bitbucket user coveo-anti-throttling_02

bitbucket user Kyu Shim (reviewer)

bitbucket user Deepak Singh (reviewer)

@olamothe

@samisayegh (reviewer)

Source: Commit e11c7ff00140 on branch KIT-76> Destination: https://bitbucket.org/coveord/ui-kit/commits/902d78eb7d87 on branch master

State: DECLINED

https://coveord.atlassian.net/browse/KIT-76

coveobot commented 4 years ago

Bitbucket user coveo-anti-throttling_02 commented on 2020-07-28 21:06

Bundle Size Report

File	Compression	Old (kb)	New (kb)	Change (%)
dist/browser/headless.js	bundled	200.4	210.1	4.8
minified	93	95.3	2.5
gzipped	27.9	28.4	1.8
dist/browser/headless.esm.js	bundled	194.3	203.8	4.9
minified	108.3	112.8	4.1
gzipped	29.8	30.5	2.3

coveobot commented 4 years ago

@samisayegh commented on 2020-07-29 14:34

Hmm, the intent behind the querySet was to store the queries located in different search boxes and keep them in sync.

What was the reason to add the other queries to this set? I believe the advanced query types should only be on the state.query object. There isn’t a need for multiple instances of aq. lq, cq, and so they should not be on the querySet.

‌

coveobot commented 4 years ago

@samisayegh commented on 2020-07-29 14:36

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

Following on the top comment, it does not make sense to me to have multiple instances of these queries. Maybe the aq if we want to support the legacy facets, but there are no plans to do so right now.

‌

coveobot commented 4 years ago

@olamothe commented on 2020-07-29 14:36

I think it makes sense to have multiple “parts”/different expression for each of those.

And so you would deal with each expression as a separate entity (and not a huge single string property).

And those parts gets merged together as a single entity in the request.

coveobot commented 4 years ago

@olamothe commented on 2020-07-29 14:40

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

I think it does :slight_smile:

Think of each of these instances as being “a specific filter I want to add”.

addAdvancedQuery(“filter for objecttype=foo”)
addAdvancedQuery(“filter for recent documents only”)
addAdvancedQuery("filter for document created by bob")

// do a query
aq=(“filter for objecttype=foo”) AND (“filter for recent documents only”) AND ("filter for document created by bob") 

// end user click on a custom checkbox in the UI to see documents from joe instead"

removeAdvancedQuery("filter for document created by bob")
addAvancedQuery("filter for document created by joe")

// do a query
aq=(“filter for objecttype=foo”) AND (“filter for recent documents only”) AND ("filter for document created by joe")

‌

coveobot commented 4 years ago

@samisayegh commented on 2020-07-29 14:40

I’m not clear on how it makes sense. Why would someone set multiple cqs, lqs in different parts of the search page? What use-case does it enable?

coveobot commented 4 years ago

@olamothe commented on 2020-07-29 14:47

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

And going even further than that, the design I have right now is relatively rough because it deals with string directly.

We will probably need something down the line to help you build valid query expression (that is, as long as search API support string query syntax instead of JSON for filters).

In JSUI we have ExpressionBuilder/QueryBuilder, and it’s been invaluable for many end users.

coveobot commented 4 years ago

@samisayegh commented on 2020-07-29 14:47

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

I see, if we wanted to support this, similar to the legacy facets, then yes we would need multiple aqs. Are there similar use-cases for the other 3 query types though?

coveobot commented 4 years ago

@olamothe commented on 2020-07-29 14:51

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

Yes. It’s 100% possible you would have different “component” in your UI that needs to independently add filter to different part of the query. Advanced or constant or disjunction is mostly a (important) difference in how they work against the index, but the same principle applies (many different pieces of loosely related code might want to add different filter in the outgoing request).

Large query is ** maybe ** different.

Normally it will be linked to just one “large text input” in your UI.

But then, we would have large query be the black sheep for no “good reason” it seems ? Like, just an exception for exception’s sake/random limitation ?

coveobot commented 4 years ago

@samisayegh commented on 2020-07-29 14:54

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

The other aspect is the querySet was not meant to be used in the query.

It was a holding-point tracking the state of search boxes. The search box that executes the search would update the query.q which would go into the request. On response, all querySet qs would be updated (not currently implemented) so all search boxes reflect the last query (similar to google.com behaviour with the sort selector).

This means that querySet.q should not be tracked for history (otherwise every key-stroke would be a snapshot).

With the aqs, they would potentially need to be tracked?

coveobot commented 4 years ago

@olamothe commented on 2020-07-29 15:00

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

I can move it to a different slice I guess ? But, adapting the history slice/feature to make it work correctly is also possible.

I just want to make clear that I think it’s very important that these filters are represented as set, otherwise I’m not sure how an end user would actually be able to interact with them in a sane manner.

If everything is just a single large string entry, you quickly get into the regex madness/index.substring territory to be able to remove/add parts to the total sum of the filter.

Any Coveo query expression is a sum of many different smaller part.

coveobot commented 4 years ago

@samisayegh commented on 2020-07-29 15:22

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

Ok, I see now how multiple aqs could be useful. This is a good addition.

On the other hand, when I see a cq, I think of a tab, so only one active instance at a time. I am less familiar with dqs and lqs, it seems like the latter is also one instance.

My view is having a dedicated set for aqs is all we need right now. Possibly better, the query slice could include multiple aqs under it to look like this:

query: {
 q: string;
 aq: Record<id, string>;
 cq: string;
 dq: string;
 lq: string;
}

The query slice has certain “attributes”: it is the source of truth for what is sent in the request (versus pulling from two different slices), and it is tracked by history. I find this structure better guides what one should do. i.e. if you need multiple queries, use aq. If multiple cqs are allowed, I might use cqs as aqs.

‌

coveobot commented 4 years ago

@olamothe commented on 2020-07-29 15:36

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

I’ll try to rephrase what I mean/what is important: A query expression is a complex string built out of many small parts. And the sum of those parts constitute the final filter applied.

That is the important concept.

And down the line, this concept will probably be changed from “a string grammar to JSON grammar” in the search API. But that sentence above (should…) remains true.

It does not matter which part (q/aq/cq/dq/lq).

‌

I find this structure better guides what one should do. i.e. if you need multiple queries, use aq. If multiple cqs are allowed, I might use cqs as aqs.

That is not true/not what the guide needs to be:

If you need dynamic filters to be considered programmatically generated, use aq.

If you need static filters to be considered programmatically generated, use cq.

If you need dynamic filters to be OR’ed with the rest of the filters and considered programmatically generated, use dq.

If you need large dynamic filters to be considered end user input, use lq.

If you need small dynamic filters to be considered end user input, use q.

‌

The fact that they are linked to one or many components in JSUI is not really relevant. The query parts exists not because of the components, but components uses them because it makes sense in regards to the above guideline.

For example, yes, Tabs use cq because they are static programmatically generated. And many clients uses cq for other things than tab.

Hopefully that’s not too confusing…

coveobot commented 4 years ago

@samisayegh commented on 2020-07-29 16:05

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

From an abstract search api point of view, the above descriptions make a lot of sense. In a real search page they may not.

If you need small dynamic filters to be considered end user input, use q.

No one needs small dynamic filters, emphasis on the “s”. You only need one. People do not search by typing half their query into one search box, the other half into another searchbox, expecting headless to assemble the two together. The querySet slice exists to solve a UI state-sync problem, not to communicate with the api. It allows to two-way bind.

Maybe the other 4 query types are different, and they need to be assembled from different places. I see this for the aq, but not the others. If there is a use-case for multiple instances of cq, lq, dq, that will help me understand.

As it is, when I consider the differences in behaviour between the q and rest (history tracking, assembling together), I feel it does not make sense to group the rest under querySet. They should be in a separate slice.

coveobot commented 4 years ago

@olamothe commented on 2020-07-29 16:22

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

For aq/cq/dq I can copy paste my same example as above with the document author=bob/joe.

The difference being :

You don’t know that the authors are named bob/joe at page load time (they change all the time), you use aq
You know the authors are named bob/joe at page load time (they never change), you use cq.
You want that expression to be applied as a OR no matter what the rest of the filters are, you use dq.

For lq, say you have a form with multiple fields that needs to be filled out for case deflection, you might want to consider each part of the form independant input for lq .

Or in insight panel, you might want to input the case description entered by the client as one part, and the case comments made by the client as another part, and the case comments made by the agent as another part etc.

‌

For q even, I would argue it makes sense.

Back in the days we supported autocompletion in the search box for query expression. Like, we’d detect you were starting to input a date, or a field name, we would try to autocomplete that entity with proper syntax for you. Like it is in slack search, if you type from: and in: , you’ll see they appear as different entity/part of the global query/highlighted as sub parts. And when you’d edit/change one part of that expression, we deal with each parts as a different entity.

That was removed because query syntax in search box causes other problems (search API does not support partial query syntax, it’s all or nothing… it’s a long discussion in itself…)

But that feature was very cool and it’s a shame we had to disable it by default.

I can move this to another slice (advanced-queries) for history tracking

Even though, as I said earlier, it would also be possible to just disregard it in history.

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-07-29 18:05

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

@{557058:6fb10fac-dc5b-4d35-b648-4b2d42952452} @{557058:ef2198d5-03b4-4d5f-86e7-0a23e51f4f14}

Ok I think I follow the conversation here, querySet was made to store/track different q from different sources e.g. search boxes, then there is a q that is “active” (q of the query slice) and that affects the rest of the state in some way (facets, results, etc.) It’s the q on the search/plan api requests basically.

I suggest we use the same kind of design, put an aq, cq, lq, dq, in both the querySet and query slices, in a string or array or object format.

The aq, cq, lq, dq in the main query slice is kind of was is sent to the API for a request, it is the active filters of the state, and they would affect it like the q does.

If there are component generating their own aq, cq, lq, dq on their side, like the search box does, we should put them in the querySet slice.

Would it make sense?

‌

coveobot commented 4 years ago

@olamothe commented on 2020-07-29 20:05

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

I can do that, yeah.

But, to be honest, I’m not even sure that splitting query-set slice and query slice is the correct thing to do, with hindsight.

We’re duplicating info in the state because it’s a bit easier to manage.

We could have done with only query-set, remove query-slice completely (keeping in mind all the previous info I’ve added above, that any part of the query should actually be considered a set/sum of multiple smaller expression), and keep something in the query set slice (a flag/a pointer) to know which one is the active search box.

Yes, there should be only one search box, we could have managed that with a pointer/flag of some sort in query-set.

We want to move most query expression to be generated/handled by the backend. But I think it is not realistic to think that it will go away completely. Whatever the query syntax ends up looking like in the search API.

A library that is essentially (currently) a search API client on steroid (with state managed for you), I think it’s essential to see handling building/managing/adding/removing valid query expression/query syntax as something we should provide first class support to.

coveobot commented 4 years ago

@samisayegh commented on 2020-07-29 20:31

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

A pointer solution would work, but it does not change the fact that there is a fundamental difference in behavior. With the q there is only one active query at a time. With an aq, there can be multiple, which are combined into a final expression.

…we would try to autocomplete that entity with proper syntax for you. Like it is in slack search …

I still see one q in this example, that is composed of multiple sub-expressions. Yes it’s possible to flatten the tree and store at the sub-expressions at the same level, with flags saying “these 3 are active”. But then you have to resolve the order … so you’d need to add an order property … that does not conflict with other q in the set? IMO, such a feature requires changing the data-structure entirely.

I do not see multiple active q at the same time making sense. The qs would be distinct expressions, unrelated to one another, and should not be combined.

coveobot commented 4 years ago

@olamothe commented on 2020-07-29 20:47

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

I’ll move it to query-slice, it’s the simplest for now, but I still think we’re just not understanding each other :wink:

‌

The merging of expression can be done in the state (in query slice), or it can be done when we do the request to the search api, like we do here with Object.values.join (history non-withstanding).

‌

There is no fundamental difference between q and aq (or cq/dq/lq).

There is a fundamental difference between a search box (that uses q) and a facet (legacy, which uses aq) yes.

You would not want multiple search box active at the same time, like you would for a facet, of course.

But q != search box, and aq != facets. It’s a means to an end/two different concepts.

But for the low level building blocks that are query expression, it’s the same.

‌

Also, the order of query expression does not matter. It’s like an addition

( A ) + ( B ) = ( B ) + ( A )

‌

Maybe the problem is just in the naming ?

Would that make more sense like this :thinking: ?

query-set.ts --> contains same thing as this pr.
search-box-set.ts -> contains the old query-set.ts logic/handle the unicity of search boxes
query-slice.ts -> becomes kind of superfluous/could be deleted

‌

coveobot commented 4 years ago

@samisayegh commented on 2020-07-29 21:12

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

A dedicated search-box-set slice would be an acceptable solution. Initially we wanted to keep higher-level UI names out of the store (since we don’t know what the UI looks like e.g. in the case of a voice interface), but we can break that rule. Switching from “pushing” the query into the query slice, to pulling from multiple slices (search-box and query-set) and assembling, means we'll need an active flag on the search-box-slice entries. I still don’t think a query-set for qs makes sense, but the separation of the two different behaviors is the important part for me.

On this comment:

A library that is essentially (currently) a search API client on steroid …

This suggests that a user of headless should be able to do any possible combination as if they were talking with the api directly. I do not see it this way. If someone wants an api client, we can generate one for them from swagger. I see headless as a library that is guiding you to do the right thing, removing the complexity of the api. It can be flexible, but also opinionated (e.g. you must enter a searchHub).

‌

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-07-29 21:25

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

A search-box slice although there will only be a simple query in it? It’s the wrong naming here IMO, a search-box is more than a query, at least the way we represent it at Coveo, it contains multiple features like querySuggest.

All of this is getting a bit confusing, we might be over-engineering this solution, could we visually see what this would look like, in a design document with a sample state with those slices with q, aq, lq, etc.

coveobot commented 4 years ago

@olamothe commented on 2020-07-29 22:35

Outdated location: line 22 of packages/headless/src/api/search/search-api-params.ts

Created a document here:

https://docs.google.com/document/d/1uK22IuzHBKQ9zl8yx2sPjLS_i0ElA4RKE9X55X-Fw-c/edit{: data-inline-card='' }

‌

coveo / ui-kit

[BB pr#90] KIT-76 feat(headless): add support for aq,cq,dq,lq #90