ORC-53. Make the complex types Comparable so they can be put as keys in OrcMap #29
Conversation
| } else if (theirs == null) { | ||
| return -1; | ||
| } else { | ||
| int val = ((Comparable<Writable>) ours).compareTo(theirs); |
There was a problem hiding this comment.
Comparing two complex types with different nested types will cause trouble here. To avoid this, we need to first compare the schemas, then the values. This applies to all the complex types.
omalley
force-pushed
the
orc-53
branch
2 times, most recently
from
77d32e7
to
62df673
Compare
|
Ok, this needed a relatively large change. To compare the schemas, I wanted to make TypeDescription implement Comparable, but the equals and hashCode were set up to match against the id, which would have largely screwed things up. So, I made SchemaEvolution use a HashMap<Integer, TypeDescription> and used the id as the key. That is more straightforward and let me change the semantics of equals and hashCode to be more reasonable. While I was in TypeDescription, I also made it Serializable since FindBugs was complaining about the OrcList and OrcMap having non-serializable fields. I also added a log4j.properties file into the test resources so that the log messages come out to the console during unit tests. I also changed the 4 complex ORC types to implement WritableComparable, which ORC-53 had done to better match with MapReduce's requirements for key types. |
| OrcStruct right = new OrcStruct(rightType); | ||
| assertEquals(-1, left.compareTo(right)); | ||
| assertEquals(1, right.compareTo(left)); | ||
| left.setAllFields(new Text("123"), new IntWritable(123)); |
There was a problem hiding this comment.
I checked out your branch and got a compilation error for setAllFields, which was introduced for ORC-52.
|
The compilation was working for me, but you had a good point that in the current version that everything should implement WritableComparable since obviously compareTo needs to recurse all the way down. Can you try the current version? |
|
Everything works on the orc-52 branch, which is based on orc-53, but with orc-53 I still see the previous error for TestOrcStruct and this new one for OrcRecordReader: |
No description provided.