time-descending result of select queries#2271
Conversation
There was a problem hiding this comment.
can we have some different names between these offsets and the one defined in the class?
There was a problem hiding this comment.
renamed offset in the class to startOffset
navis
force-pushed
the
descending-for-select-query
branch
from
5d99df4 to
63d2213
Compare
|
👍 @navis please add documentation for select query that it is reversible |
navis
force-pushed
the
descending-for-select-query
branch
from
340617c to
5caaf9b
Compare
|
updated document |
There was a problem hiding this comment.
I see other methods in this class violate "normal" bean naming conventions. Oh well...
There was a problem hiding this comment.
@drcrallen any other comments? I think this is pretty close to ready
There was a problem hiding this comment.
Also I think in Druids and in other places, we did not follow the 'set' convention in most of the builder classes.
|
@himanshug can you take a look? |
There was a problem hiding this comment.
This is a part of the java spec I'm not as familiar with. Is this doing ((descending ? 1 : 0) ^ value) < 0 or descending ^ (value < 0)?
There was a problem hiding this comment.
IntelliJ says it's descending ^ (value < 0). I'll add clarifying parentheses.
navis
force-pushed
the
descending-for-select-query
branch
from
5caaf9b to
b1e3923
Compare
There was a problem hiding this comment.
this is so much tidier now
|
Functionality looks good. Have some outstanding questions about docs and possibly some more unit tests. |
|
Also please squash soon, this is very close to ready |
|
@navis can you squash commits? |
|
squashed |
|
@drcrallen any more comments? |
|
is not squashed :) did @navis forget to push? |
|
@navis ? |
|
Ah, sorry. I've confused with other issue. |
navis
force-pushed
the
descending-for-select-query
branch
from
8301f47 to
55a888e
Compare
|
👍 |
time-descending result of select queries
5 participants
#2014 for select query