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

Rework sorting by multiple attributes #11

Closed
1 task done
novoj opened this issue Feb 24, 2023 · 5 comments · Fixed by #149
Closed
1 task done

Rework sorting by multiple attributes #11

novoj opened this issue Feb 24, 2023 · 5 comments · Fixed by #149
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@novoj
Copy link
Collaborator

novoj commented Feb 24, 2023

We need to avoid and remove data type Multiple. Problem with this type is that it hides inner data types (which there may be multiple) and there is also problem how to describe this type to the end users. We need to stick to analogies to existing and well established data store - primarily SQL. On the other hand we need to allow both these scenarios:

  • sort by two or more attributes in "expected way" - i.e. secondary ordering decides order of elements when primary ordering cannot decide because values are the same
  • sort by two or more attributes when the sorting handles "NULLs" last scenario - i.e. there is no attribute found for primary attribute in an entity

We also want to avoid situation where we would need to compare values of the entities in real-time. We already know it's slower than current masking process for pre-sorted arrays. This was the original reason why Multiple constraint exists in the first place.

The proposed solution discussed with @lho is:

  • add support for creating multi-attribute sort indexes in EntitySchema / Reference schema by adding so called SortableAttributeCompounds which will aggregate sort direction for multiple existing attributes (which doesn't need necessarily to be sortable themselves)

The final composition would like this:

attribute(nameOfCompound, DESC)
  • we should also relax on requiring that sortable reference attribute is present in entity only once, when multiple references are selected the sort order should be combined:
    • in case of hierarchy reference by a deep traversal order of the filter matching hierarchy tree
    • in case of plain referenced entities by a ascending order of their primary keys
@novoj novoj added the enhancement New feature or request label Feb 24, 2023
@novoj novoj added this to the Alpha milestone Feb 24, 2023
@novoj novoj self-assigned this Feb 24, 2023
novoj added a commit that referenced this issue Jun 6, 2023
Removed `Multiple` type, added support for multiple arguments in `attributeNatural` ordering constraint.
@lukashornych
Copy link
Collaborator

In order to support multiple attribute names in order constraints we will not use classifiers for it, instead we use value annotation. But this way we cannot have 2 same constraints on the same level. Thus, we introduce array into orderBy with implicit container but unlike in filterBy where we use AND for it, here we just flat children into parent container.

{
  orderBy: [
	{
	  "attributeCodenatural": DESC,
	  "attributeNatural": {}
	},
	{
	  "attributeNatural": {},
	  "entityproperty": {

	  }
	}
  ]
}

novoj added a commit that referenced this issue Jun 7, 2023
Removed `Multiple` type, added support for multiple arguments in `attributeNatural` ordering constraint.
lukashornych added a commit that referenced this issue Jun 9, 2023
…ish support for multiple creators with same name and different classifiers
novoj added a commit that referenced this issue Jun 23, 2023
We need to avoid and remove data type `Multiple`. Problem with this type is that it hides inner data types (which there may be multiple) and there is also problem how to describe this type to the end users. We need to stick to analogies to existing and well established data store - primarily SQL. On the other hand we need to allow both these scenarios:

- sort by two or more attributes in "expected way" - i.e. secondary ordering decides order of elements when primary ordering cannot decide because values are the same
- sort by two or more attributes when the sorting handles "NULLs" last scenario - i.e. there is no attribute found for primary attribute in an entity

We also want to avoid situation where we would need to compare values of the entities in real-time. We already know it's slower than current masking process for pre-sorted arrays. This was the original reason why `Multiple` constraint exists in the first place.

The proposed solution discussed with @lho is:

- add support for creating multi-attribute sort indexes in EntitySchema  / Reference schema by adding so called SortableAttributeCompounds which will aggregate sort direction for multiple existing attributes (which doesn't need necessarily to be sortable themselves)

The final composition would like this:

```
attribute(nameOfCompound, DESC)
```
novoj added a commit that referenced this issue Jun 24, 2023
We should also relax on requiring that sortable reference attribute is present in entity only once, when multiple references are selected the sort order should be combined:
   - in case of hierarchy reference by a deep traversal order of the filter matching hierarchy tree
   - in case of plain referenced entities by a ascending order of their primary keys
novoj added a commit that referenced this issue Jun 24, 2023
…indexed`

Indexed name much more captures the meaning of this property - it controls not only whether the reference is filterable, but also whether it is sortable.
@novoj
Copy link
Collaborator Author

novoj commented Jun 24, 2023

Implemented by my side. I'd need help from @lho here - see TODOs in commit ac201f5

@novoj
Copy link
Collaborator Author

novoj commented Jun 25, 2023

I've added two additional TODOs for missing tests

novoj added a commit that referenced this issue Jun 25, 2023
novoj added a commit that referenced this issue Jun 26, 2023
novoj added a commit that referenced this issue Jun 26, 2023
@lukashornych
Copy link
Collaborator

TODOs that were on me are done.

@novoj
Copy link
Collaborator Author

novoj commented Jun 27, 2023

Cool, tests are passing, we're finally done.

@novoj novoj linked a pull request Jun 27, 2023 that will close this issue
novoj added a commit that referenced this issue Jun 27, 2023
…attributes

#11 rework sorting by multiple attributes
novoj added a commit that referenced this issue Jun 27, 2023
lukashornych added a commit that referenced this issue Jun 30, 2023
novoj pushed a commit that referenced this issue Jul 3, 2023
novoj added a commit that referenced this issue Sep 20, 2023
According to the test results we observer a great reduction in throughput due to ordering. Introduced in #11
novoj added a commit that referenced this issue Sep 21, 2023
According to the test results we observer a great reduction in throughput due to ordering. Introduced in #11
novoj added a commit that referenced this issue Sep 21, 2023
According to the test results we observer a great reduction in throughput due to ordering. Introduced in #11
novoj added a commit that referenced this issue Sep 21, 2023
According to the test results we observer a great reduction in throughput due to ordering. Introduced in #11
novoj added a commit that referenced this issue Sep 21, 2023
perf(#263): Performance regression in ordering

According to the test results we observer a great reduction in throughput due to ordering. Introduced in #11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants