Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ECP-469: Finalize new search schema #368
ECP-469: Finalize new search schema #368
Changes from all commits
ac246a9
ed8109c
c1171ba
3aa8567
1ca6590
8456844
dc48ac3
113b8e2
89f26e8
e085961
5d77acf
9d3fa57
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
productSearch
should be nullable, or all other operations underQuery
in a request will fail. See #372There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why should we have direction as a mandatory attribute?
Most of RDBMS just use a default value of ASC / DESC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is official elastic search documentation:
I don't like this uncertainty
ProductSearchSortInput
is optional so we can apply some default order, but if developer asks for custom ordering I think it's better to ask for direction as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like @maghamed's idea of having a default. If something isn't critical for an operation to succeed, (I think) it should be optional. Otherwise querying becomes tedious and verbose.
I don't see much uncertainty from that quote, if we're only talking about ES.
But, are we planning to only support Elastic Search? If we're ever going to swap search providers, I think it would make sense for the application to define the default so it's consistent.
I think an application-defined default sort order would be great 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no need for any changes. We have the optional
ProductSearchSortInput
, so we can apply default sort. However, the user needs to specify direction for every custom sort. Does it work?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent field name,
attribute_code
here vs.attribute
on other type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed with @kokoc verbally, I think
facets
is sufficient for storing facets aggregation and I am not clear the use case forfacet_values
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a common convention to use camelCase for field name (e.g. matchedWords instead of matched_words). Not sure if this is Magento convention or else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All fields under the interface (e.g. Bucket) must be listed under any type that implements the interface. e.g. I don't see
title
listed under StatsBucket, ScalarBucket and RangeBucket.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be a
type
instead of aninterface
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add
title
field for storing aggregation friendly name. This is so the client (e.g. UI) can use it for display, instead of displaying attribute code.