Skip to content

Add common utility function ScanUtil.splityKeyRangesByBoundaries() #485

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.

Sign up for GitHub

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

Jump to bottom
Merged
twdsilva merged 3 commits into from
Apr 19, 2019
Merged

Add common utility function ScanUtil.splityKeyRangesByBoundaries() #485

twdsilva merged 3 commits into from
Apr 19, 2019

Conversation

@binshi-bing
Copy link
Contributor

@binshi-bing binshi-bing commented Apr 16, 2019

What does this PR do?

Which problem(s) does it fix and how? What does this PR add?

Where should the reviewer start?

Explain what should be set-up first in order to get the review started. Which page has to be viewed etc.

How should this be manually tested?

  • Write down all
  • the steps a person
  • needs to take to test it.

Documents

Screenshots (if appropriate)

If this PR changes something that concerns UI changes, please post before and after screenshots here.

Other documents

If you have any documents that have anything to do with this PR.

Who should be notified?

  • Write down the name/department this branch needs
  • to be communicated to
  • Check the box(es) to indicate that it has been
  • communicated (add the label)

What should happen on deployment? (check all that apply)

  • The usual steps.
  • There are database changes, which should be run.
  • There are files that should be manually uploaded.

Questions:

  • Does this require a blog post?
  • Does this require a knowledge base update?
  • Does support need training for this?
  • Does this has to be communicated to partners?

Bin added 2 commits April 8, 2019 15:57
PHOENIX-5176 KeyRange.compareUpperRange(KeyRang 1, KeyRang 2) returns…
eeec239 
… wrong result when two key ranges have the same upper bound values but one is inclusive and another is exclusive
Merge remote-tracking branch 'upstream/phoenix-stats' into phoenix-stats
69a4e8f
@binshi-bing
Copy link
Contributor Author

binshi-bing commented Apr 16, 2019

@twdsilva @dbwong @yanxinyi

dbwong
dbwong reviewed Apr 17, 2019
View reviewed changes
dbwong
dbwong approved these changes Apr 17, 2019
View reviewed changes
Copy link
Contributor

@dbwong dbwong left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, sorry I wasn't clear what I meant when i said boundaries couldn't be size 0. What I meant was that the elements of boundaries, the individual byte[]'s those cannot be length 0. And maybe add a comment to that.

@binshi-bing
Copy link
Contributor Author

binshi-bing commented Apr 17, 2019

Looks good, sorry I wasn't clear what I meant when i said boundaries couldn't be size 0. What I meant was that the elements of boundaries, the individual byte[]'s those cannot be length 0. And maybe add a comment to that.

OK, I'll a comment.

twdsilva
twdsilva reviewed Apr 18, 2019
View reviewed changes
Copy link
Contributor

@twdsilva twdsilva left a comment

Choose a reason for hiding this comment

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

Nice work, just had one minor comment.

phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java Outdated
* @param keyRanges
* The key ranges to split along the given boundaries. Coalesced.
* @return
* List<Pair<RangeIndex, Query Key Range List in this range>>.
Copy link
Contributor

@twdsilva twdsilva Apr 18, 2019

Choose a reason for hiding this comment

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

should this be List<Pair<BoundaryIndex, Query Key Range List in this boundary>> ?

Copy link
Contributor Author

@binshi-bing binshi-bing Apr 19, 2019

Choose a reason for hiding this comment

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

@twdsilva , N boundaries split the key space into N+1 ranges. Here each pair is the index of of such a range and the query key ranges belongs to that range. Probably still use <RangeIndex, Query Key Range List in the range>? Let me add more comment and you can decide how it will be more accurate.

@twdsilva
Copy link
Contributor

twdsilva commented Apr 19, 2019

+1, can you please squash your changes and I will get this committed.

Add common utility function ScanUtil.splityKeyRangesByBoundaries().
7a1ab14 
Change the return type of splitKeyRangesByBoundaries() from List<KeyRange> to List<Pair<Integer, List<KeyRange>>>.
Handled feedback.
@binshi-bing
Copy link
Contributor Author

binshi-bing commented Apr 19, 2019 •
edited
Loading

@twdsilva, just squashed. Could you check what I did is correct?

@twdsilva twdsilva merged commit b16ef0a into apache:phoenix-stats Apr 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@twdsilva twdsilva twdsilva left review comments

@dbwong dbwong dbwong approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@binshi-bing @twdsilva @dbwong