-
Notifications
You must be signed in to change notification settings - Fork 521
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
Improve query performance with HBase #2178
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2178 +/- ##
============================================
- Coverage 68.66% 65.08% -3.59%
Complexity 977 977
============================================
Files 497 497
Lines 40564 40574 +10
Branches 5662 5664 +2
============================================
- Hits 27855 26407 -1448
- Misses 10015 11551 +1536
+ Partials 2694 2616 -78
... and 69 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
short value = (short) (((startRow[0] << 8) | (startRow[1] & 0xFF)) +1); | ||
byte[] endRow = ByteBuffer.allocate(2).putShort(value).array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not quiet clear with it,could u explain it more?
and for any of the scan query condition,we always add the end-row?
could u give a case for it(before vs after),and check the ci problems(a string of ci failed now)
@@ -418,7 +419,11 @@ default R scan(String table, Set<byte[]> prefixes) { | |||
*/ | |||
default R scan(String table, byte[] startRow, boolean inclusiveStart, | |||
byte[] prefix) { | |||
short value = (short) (((startRow[0] << 8) | (startRow[1] & 0xFF)) +1); | |||
byte[] endRow = ByteBuffer.allocate(2).putShort(value).array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can call increaseOne()
?
Scan scan = new Scan().withStartRow(startRow, inclusiveStart) | ||
.setFilter(new PrefixFilter(prefix)); | ||
Scan scan = new Scan(); | ||
if (table.equals("g_oe") || table.equals("g_ie")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverse the equals, and seems we could use the Constant
vars to avoid hard code? (search it)
if (table.equals("g_oe") || table.equals("g_ie")) { | |
if ("g_oe".equals(table) || "g_ie".equals(table)) { |
@javeme @zyxxoo we could use suggestion
for users to know the review clearly (and could also apply it directly)
scan.withStartRow(startRow, inclusiveStart) | ||
.setFilter(new PrefixFilter(prefix)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use one line if ≤ 100
.setFilter(new PrefixFilter(prefix)); | ||
Scan scan = new Scan(); | ||
if (table.equals("g_oe") || table.equals("g_ie")) { | ||
short value = (short) (((startRow[0] << 8) | (startRow[1] & 0xFF)) +1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format the code +1
(lack a space)
Scan scan = new Scan().withStartRow(startRow, inclusiveStart) | ||
.setFilter(new PrefixFilter(prefix)); | ||
Scan scan = new Scan(); | ||
if (table.equals("g_oe") || table.equals("g_ie")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a method HbaseTables.Edge.isEdgeTable(table)
.setFilter(new PrefixFilter(prefix)); | ||
Scan scan = new Scan(); | ||
if (table.equals("g_oe") || table.equals("g_ie")) { | ||
short value = (short) (((startRow[0] << 8) | (startRow[1] & 0xFF)) +1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add some comments for why?
Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label |
@JackyYangPassion address this PR? |
Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label |
Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label |
Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label |
Is it still in progress? |
Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label |
Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label |
Thank you everyone for reviewing and giving this PR #2364 to fix in the future. |
fix #2177