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

#1264 Add .range() method to sorted_map and sorted_set #1359

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

E-M-T-F
Copy link

@E-M-T-F E-M-T-F commented Dec 24, 2024

Added .range() method to sorted_map and sorted_set, and privode the test cases

Copy link

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are three potential issues I've identified in the provided git diff output:

1. Inclusive vs. Exclusive Range in Documentation

  • In the sorted_map/map.mbt file, the documentation comment for the range function states:
    ///|Returns a new array of key-value pairs that are within the specified range [low, high).
    
    However, the implementation of the range function uses k >= low && k <= high, which includes both low and high in the range. This is an inclusive range, not an exclusive one as suggested by the documentation.
    • Suggestion: Update the documentation to reflect the inclusive nature of the range, or modify the implementation to match the documentation.

2. Potential Performance Issue in range Function

  • In both sorted_map/map.mbt and sorted_set/set.mbt, the range function iterates over all elements in the map or set and checks if each element falls within the specified range. This approach is inefficient for large collections because it doesn't take advantage of the sorted nature of the data structure.
    • Suggestion: Implement a more efficient range query that leverages the sorted order of the keys. For example, you could use a binary search to find the starting and ending points of the range and then iterate only over the relevant subset of the data.

3. Typo in sorted_map/map_test.mbt

  • In the range test for the sorted_map/map_test.mbt file, the expected content is:
    content=
      #|[(2, "b"), (3, "c"), (4, "d")]
    
    However, the actual call to map.range(2, 4) should only include elements with keys 2, 3, and 4. The expected result should not include (4, "d") because the range is [2, 4), which is exclusive of 4.
    • Suggestion: Update the test to reflect the correct expected result:
      content=
        #|[(2, "b"), (3, "c")]
      

These are the three main issues I've identified. Addressing these will improve the correctness, performance, and clarity of your code.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 4305

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 81.268%

Totals Coverage Status
Change from base Build 4303: 0.01%
Covered Lines: 4629
Relevant Lines: 5696

💛 - Coveralls

@bobzhang
Copy link
Contributor

the implementation looks good to me, but this is not commonly used operation, shall we leave it in the user land?

@E-M-T-F
Copy link
Author

E-M-T-F commented Dec 25, 2024

Related: #1264

@E-M-T-F
Copy link
Author

E-M-T-F commented Dec 25, 2024

This PR is the solution of issue #1264, so I think it is an useful solution for the user land

@bobzhang
Copy link
Contributor

@E-M-T-F that looks good to me, can you change the API to return Iter2 instead?

range[K : Compare, V](Self[K, V], K, K) -> Iter2[K,V]

@E-M-T-F
Copy link
Author

E-M-T-F commented Dec 26, 2024

@bobzhang OK,I will change the API to return Iter2 instead

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.

3 participants