Skip to content

Conversation

@NathanQingyangXu
Copy link
Contributor

@NathanQingyangXu NathanQingyangXu commented Feb 12, 2020

@NathanQingyangXu NathanQingyangXu force-pushed the SortNatual-by-default-for-Sorted branch from 0fed650 to a0061d3 Compare February 12, 2020 03:59
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I manually compared to explicit @SortNatural case and found everything is the same(only hadExplicitSort values are different but it is used internally in this method and its main usage is the above code snippet), so removing the above code snippet seems enough to achieve the goal of this PR, unless I were wrong.

@dreab8 dreab8 added the 6.0 label Feb 14, 2020
@NathanQingyangXu NathanQingyangXu changed the title Support for sorted-set and sorted-map without @SortNatural or @SortComparator Wip/6.0 Support for sorted-set and sorted-map without @SortNatural or @SortComparator Feb 14, 2020
@sebersole
Copy link
Member

I need to look through this some more.

However, this would be a new feature or improvement since we did not previously support this. New features/improvements ought to have a corresponding Jira - the reason being it will be easier to consolidate all new features and improvements when we get to CRs and eventually Final

Copy link
Member

Choose a reason for hiding this comment

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

We should also assert that the phones are in fact sorted in proper order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Originally I had thought we might end up with testing SortedSet or SortedMap. Testing case enriched enormously.

@NathanQingyangXu NathanQingyangXu force-pushed the SortNatual-by-default-for-Sorted branch 2 times, most recently from a05f49f to 337fe8f Compare February 23, 2020 16:30
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make sense to insert testing data with the natural order. Ideally we are supposed to populate big number of data to ensure the order is not maintained not by accident.

@NathanQingyangXu NathanQingyangXu force-pushed the SortNatual-by-default-for-Sorted branch from 337fe8f to d00d8cb Compare February 23, 2020 16:45
@NathanQingyangXu NathanQingyangXu changed the title Wip/6.0 Support for sorted-set and sorted-map without @SortNatural or @SortComparator Wip/6.0 HHH-13877 Make@SortNatural as default for SortedSet and SortedMap Feb 23, 2020
@NathanQingyangXu
Copy link
Contributor Author

I need to look through this some more.

However, this would be a new feature or improvement since we did not previously support this. New features/improvements ought to have a corresponding Jira - the reason being it will be easier to consolidate all new features and improvements when we get to CRs and eventually Final

I created a ticket for this: https://hibernate.atlassian.net/browse/HHH-13877 and updated the PR accordingly.

@NathanQingyangXu NathanQingyangXu changed the title Wip/6.0 HHH-13877 Make@SortNatural as default for SortedSet and SortedMap Wip/6.0 HHH-13877 Make@SortNatural default for SortedSet and SortedMap Feb 23, 2020
Copy link
Member

@sebersole sebersole left a comment

Choose a reason for hiding this comment

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

Looked through this some more and approve the changes.

However because of other "code cleanup" type changes a few files here now have completely unnecessary conflicts which is the problem I was pointing out a few weeks ago.

Anyway, please rebase these changes on top of those cosmetic cleanups. Thanks

@NathanQingyangXu NathanQingyangXu force-pushed the SortNatual-by-default-for-Sorted branch from d00d8cb to 7648230 Compare March 19, 2020 19:03
@NathanQingyangXu
Copy link
Contributor Author

Looked through this some more and approve the changes.

However because of other "code cleanup" type changes a few files here now have completely unnecessary conflicts which is the problem I was pointing out a few weeks ago.

Anyway, please rebase these changes on top of those cosmetic cleanups. Thanks

A lesson learned. Conflict resolved (mainly from another PR, but yeah, cosmetic code changes break other PRs easily) and force pushed.

@sebersole
Copy link
Member

Thanks @NathanQingyangXu

I'll apply after the checks complete

@sebersole sebersole merged commit 7e2987a into hibernate:wip/6.0 Mar 19, 2020
@NathanQingyangXu NathanQingyangXu deleted the SortNatual-by-default-for-Sorted branch March 19, 2020 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants