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

Date field sorting #384

Merged
merged 3 commits into from
Apr 20, 2023
Merged

Date field sorting #384

merged 3 commits into from
Apr 20, 2023

Conversation

maheshrajamani
Copy link
Contributor

@maheshrajamani maheshrajamani commented Apr 20, 2023

What this PR does:
Added sorting for date field.

Which issue(s) this PR fixes:
Fixes #374

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

@maheshrajamani maheshrajamani changed the title Date field sort Date field sorting Apr 20, 2023
@maheshrajamani maheshrajamani marked this pull request as ready for review April 20, 2023 13:43
@maheshrajamani maheshrajamani requested a review from a team as a code owner April 20, 2023 13:43
@tatu-at-datastax
Copy link
Contributor

tatu-at-datastax commented Apr 20, 2023

Mostly looks good, but I think this is missing changes to JsonNodeComparator; it would be counting on sorting by Numbers, overlapping with Number type vs Date; and I think this is not fully correct. We may need to discuss this further.
It sort of kind of works, for actual usage. But I think Mongo would separate Date values from Numbers (would not mix entries with column values of Date and Number types).

I know that our in-memory sorting is expected to be temporary, and replaced by global sort so this may be acceptable.
But depends on how closely we want to implement Mongo-compatible logic.
I think for most users sorting numerically would work ok tho; mixing Numbers and Dates is probably not a common use case (or other mixing; precedence of Date vs other types is different from that of Number)

Now: to fully implement this, JsonNodeComparator would I think need to auto-detect Date from JsonNode.

@maheshrajamani
Copy link
Contributor Author

Now: to fully implement this, JsonNodeComparator would I think need to auto-detect Date from JsonNode.

As per discussion with @tatu-at-datastax, have used PojoNode for Date type.

@tatu-at-datastax
Copy link
Contributor

Now: to fully implement this, JsonNodeComparator would I think need to auto-detect Date from JsonNode.

As per discussion with @tatu-at-datastax, have used PojoNode for Date type.

This works beautifully since it can be reliably created from query_timestamp_values (no need for EJSON encoding) and sorts exactly as per spec. Good job!

@maheshrajamani maheshrajamani merged commit 4dc6d04 into main Apr 20, 2023
@maheshrajamani maheshrajamani deleted the date-sort branch April 20, 2023 17:51
.hasSize(5)
.isEqualTo(
List.of(
objectMapper.readTree(doc1),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend adding another query to this test that sorts in reverse order, and asserts that sorting by new FindOperation.OrderBy("sort_date", false) returns doc6, doc5, doc4, doc3, doc2. This test as written might succeed even if no sorting was done, because the documents happen to be in ascending order by sort_date.

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

One minor comment but LGTM overall

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Date/Time support) Support sorting on Date/Time valued fields
3 participants