alejandro-anadon commented on PR #1458:
URL: https://github.com/apache/phoenix/pull/1458#issuecomment-1183253538
Thanks for the time and atention.
yes. Your summary is correct.
Just to point out that I also added a UUID ARRAY type, but not a
UUID_INDEXABLE ARRAY because I did not see a use case.
I would like to see a few points:
1) For the UUIDIndexable I tried to develop an algorithm (more complex than
a simple .toString()) to make the UUID more compact.
And I succeeded. It stayed at 16 bytes if there was no 0x00 or 0xFF or at 19
bytes if there was any 0x00 or 0xFF. It worked.
But I had to reject it because it lost the (byte) order within the hbase
when it is part of the primary key
making the "select" with an "order by" not work.
With the .tosTring() I get that there is neither 0x00 nor 0xFF and also the
order is maintained (but it is less compact and perhaps slower).
If you see fit I can go deeper in the explanation of this algorithm and
attach it but I think that having rejected it is not appropriate to do it here.
2) Regarding the descending vs ascending problem, I could solve it (or so I
think).
In my first commit I didn't (mistakenly) take into consideration that the
keys or indexes could have a DESC; then everything
worked until I started to use the DESC. And I didn't understand why.
After some research (the only "documentation" I have of internals os Phoenix
is the source code; and I had to do a lot of try and failure)
I realized that in the Hbase, when using DESC, the bytes are inverted.
That's when I corrected the algorithm so that it would work also when using
when using DESC. then I commited the changes.
3) There is something I may be doing wrong:.
in the method
org.apache.phoenix.expression.function.StringToUUIDFunction.evaluate(Tuple
tuple, ImmutableBytesWritable ptr)
I am doing this check:
"
String value;
if (ptr.get()[ptr.getOffset() + 8] == '-') {
value =new String(ptr.get(), ptr.getOffset(),
UUID_TEXTUAL_LENGTH,Charset.forName("UTF-8"));
} else {
byte[] temp =SortOrder.invert(ptr.get(), ptr.getOffset(), new
byte[36], 0, UUID_TEXTUAL_LENGTH);
value = new String(temp, Charset.forName("UTF-8"));
}
"
Why this " == '-' " check??
because when writing the tests, in the test
org.apache.phoenix.expression.function.UUIDFunctionTest.testUUIDFuntions() I
have two checks:
A) to watch over the ASC
UUID uuid = UUID.randomUUID();
String uuidStr = testUUIDToStringFunction(uuid, null, SortOrder.ASC);
UUID returnUUid = testStringToUUIDfunction(uuidStr, null,
SortOrder.ASC);
assertTrue(uuid.equals(returnUUid));
B) to watch over the DESC:
uuid =UUID.randomUUID();
uuidStr = testUUIDToStringFunction(uuid, null, SortOrder.DESC);
returnUUid = testStringToUUIDfunction(uuidStr, null,
SortOrder.DESC);
assertTrue(returnUUid.equals(UUID.fromString(uuidStr)));
When it is ASC, there is no problem, but in DESC, if I don't make that check
(in StringToUUIDFunction), and I use directly what is in 'ptr'
it gives error because the bytes are inverted and I have to invert them
again.
If I don't make the StringToUUIDFunction check of " == '-' " and use
directly what is in the ptr, all the other
tests (disabling the DESC test in testUUIDFuntions) work correctly.
This makes me think that what is happening is that I have badly designed
the DESC test in UUIDFunctionTest
and I am "adapting" the function StringToUUIDFunction only for that test
and that in real life
it would never happen that: that StringToUUIDFunction never arrives the
'ptr' with the inverted bytes.
I think that in order for that to happen, there should be someting like:
select STR_TO_UUID('uuid-string-with-bytes-inverted');
And that makes no sense.
So, what do you think? Should I remove the StringToUUIDFunction validation
of " == '-' " and remove the DESC test?
Can it be that the DESC test in UUIDFunctionTest is badly designed?
4) Is this logic correct ? (this is what now it is implemented):
assertTrue(PUUID.INSTANCE.isCoercibleTo(PUUIDIndexable.INSTANCE));
assertFalse(PUUIDIndexable.INSTANCE.isCoercibleTo(PUUID.INSTANCE));
or it should be:
assertTrue(PUUID.INSTANCE.isCoercibleTo(PUUIDIndexable.INSTANCE));
assertTrue(PUUIDIndexable.INSTANCE.isCoercibleTo(PUUID.INSTANCE));
5) sqlTypes I had put:
PUUID -> 2100
PUUIDIndexable -> 2101
should be other numbers? (easy to change because I grouped all numbers in
PDataType)
6) In the Apache Yetus(jenkins) errors report (which appears and disappears
like a ghost), I am seeing strange things:
A) error: unit : it is reporting "refCount leaked" many times; And
this error seems to be a random thing, because
in the report that I get two days ago coincides with the
"refCount leaked" but in totally different places.
Also the tests that I run locally pass the tests successfully.
B) error: checkstyle:
Type error 1. Instantiation of String. There are 5 instances, but this is a
sample:
"p.e.
./phoenix-core/src/main/java/org/apache/phoenix/expression/function/StringToUUIDFunction.java:87:
value = new String(temp, Charset.forName("UTF-8"));
Instantiation of java.lang.String should be avoided.
[IllegalInstantiation]"
Is it a checkstyle error?
(https://github.com/checkstyle/checkstyle/issues/2404 )
If not, how can I instate a String ?
Type error 2: Checkstyle WhitespaceAround Errors in places where I didn't
made changes:
"p.e.
./phoenix-core/src/main/java/org/apache/phoenix/util/IndexUtil.java:635:
if(skipWAL) {:9:
'if' is not followed by whitespace. [WhitespaceAround]"
Ok; there is a WhitespaceAround error... but, It comes from before I didn't
touch it. Should I correct it?
My way of working is to try to change as little as possible
and I think it would be worse if I change it.
But if it is convenient to change it, I change it.
Type error 3: Checkstyle Indentation Errors. Lines 95,96 and 97:
"p.e.
./phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataTypeFactory.java:
95,96 and 97
types.add(PUUID.INSTANCE);:
'ctor def' child has incorrect indentation level 4,
expected level should be 8. [Indentation]"
The lines before and after are indentation level 4; not 8. So if I indent to
8, it will be something like:
...
types.add(PUnsignedTinyint.INSTANCE);
types.add(PUnsignedTinyintArray.INSTANCE);
types.add(PUUID.INSTANCE); <-- this line will be over indentation. Can't
show it in this preview window
types.add(PUUIDArray.INSTANCE); <-- this line will be over indentation.
Can't show it in this preview window
types.add(PUUIDIndexable.INSTANCE); <-- this line will be over indentation.
Can't show it in this preview window
// is it necessary to build a PUUIDIndexableArray type ? Now I don't find
any case
types.add(PVarbinary.INSTANCE);
types.add(PVarbinaryArray.INSTANCE);
...
What should I do? left it as level 4 or change all the
file to level 8 ?
I prefer to left it as level 4 (minimun changes, but
Checkstyle Indentation Error)
C) misconception or misunderstanding:
in PUUID , PUUIDIndexable and PUUIDArray: should I implement the functions
'hashCode()' and/or 'equals()'?
I have see that in the other types this is not done; so I
didn't implement them. But if I don't do it I have
a "spotbugs" that says that I have to do it; (the report of two
days ago was removed from the pull request history,
so I don't know what is the exact description is)
Why does it give this error in these new types and in the
others it doesn't give the error?
And the main question: do I have to implement the functions
'hashCode()' and/or 'equals()'?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
