Python 3 support

[wbolster]

This would be trivial for HappyBase, but the underlying Thrift library needs to be Python 3 compatible first.

A simple '2to3' on the source code seems to work, but this needs to be properly supported in a stable Thrift release first.

Status/todo for this issue:

• Merge Enable building universal wheels #78
• Regenerate Thrift bindings using a newer Thrift version (maybe not needed)
• Update docs (TODO.rst)

[wbolster]

Thift seems to be Python 3 compatible nowadyas. See also #78.

[wbolster]

(removed comment, now part of issue description)

[jeroenvlek]

Thrift is getting there: https://issues.apache.org/jira/browse/THRIFT-1857.

I'm highly interested in this patch. Please let me know if there's anything I can do to help. For now, I'll just fork this, apply #78 and regenerate the bindings with a nightly from Thrift.

[jeroenvlek]

Ugh, I didn't actually want to link these two issues, but I just wanted to indicate where I moved my comments...

Anyway, so I used Thrift 1.0.0-dev to gain Python 3 support and one of the tests fails:

======================================================================
ERROR: tests.test_api.test_atomic_counters
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jvlek/.virtualenvs/happybase/lib/python3.4/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/jvlek/dev/lib/happybase/tests/test_api.py", line 172, in test_atomic_counters
    table.counter_set(row, column, 0)
  File "/home/jvlek/dev/lib/happybase/happybase/table.py", line 540, in counter_set
    self.put(row, {column: pack_i64(value)})
  File "/home/jvlek/dev/lib/happybase/happybase/table.py", line 436, in put
    batch.put(row, data)
  File "/home/jvlek/dev/lib/happybase/happybase/batch.py", line 132, in __exit__
    self.send()
  File "/home/jvlek/dev/lib/happybase/happybase/batch.py", line 55, in send
    self._table.connection.client.mutateRows(self._table.name, bms, {})
  File "/home/jvlek/dev/lib/happybase/happybase/hbase/Hbase.py", line 1598, in mutateRows
    self.send_mutateRows(tableName, rowBatches, attributes)
  File "/home/jvlek/dev/lib/happybase/happybase/hbase/Hbase.py", line 1607, in send_mutateRows
    args.write(self._oprot)
  File "/home/jvlek/dev/lib/happybase/happybase/hbase/Hbase.py", line 7470, in write
    iter351.write(oprot)
  File "/home/jvlek/dev/lib/happybase/happybase/hbase/ttypes.py", line 589, in write
    iter6.write(oprot)
  File "/home/jvlek/dev/lib/happybase/happybase/hbase/ttypes.py", line 495, in write
    oprot.writeString(self.value)
  File "/home/jvlek/.virtualenvs/happybase/lib/python3.4/site-packages/thrift/protocol/TBinaryProtocol.py", line 122, in writeString
    encoded = bytearray(str, 'utf-8')
nose.proxy.TypeError: encoding or errors without a string argument

Which simplifies to:

>>> from struct import Struct              
>>> bytearray(Struct('>q').pack(0), 'utf-8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: encoding or errors without a string argument

This is due to a new line in TBinaryProtocol that wasn't there before:

It seems that the 'utf-8' encoding parameter in TBinaryProtocol causes the problems, which is new in 1.0.0-dev:

## Thrift 0.9.2: lib/py/src/protocol/TBinaryProtocol.py ## 
def writeString(self, str):
  self.writeI32(len(str))
  self.trans.write(str)

## Thrift 1.0.0-dev: lib/py/src/protocol/TBinaryProtocol.py ## 
def writeString(self, str):
  encoded = bytearray(str, 'utf-8')
  self.writeI32(len(encoded))
  self.trans.write(encoded)

The integer probably should be send as a TType.I64. Does that mean a new message type, because it can't be send as part of a mutateRows_args anymore? Or is there a way to enforce a different datatype with mutations? edit: No, in ttypes.Mutation.thrift_spec a value is defined as TType.STRING

Furthermore, the tearDown fails:

======================================================================
ERROR: test suite for <module 'tests.test_api' from '/home/jvlek/dev/lib/happybase/tests/test_api.py'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jvlek/.virtualenvs/happybase/lib/python3.4/site-packages/nose/suite.py", line 229, in run
    self.tearDown()
  File "/home/jvlek/.virtualenvs/happybase/lib/python3.4/site-packages/nose/suite.py", line 352, in tearDown
    self.teardownContext(ancestor)
  File "/home/jvlek/.virtualenvs/happybase/lib/python3.4/site-packages/nose/suite.py", line 368, in teardownContext
    try_run(context, names)
  File "/home/jvlek/.virtualenvs/happybase/lib/python3.4/site-packages/nose/util.py", line 471, in try_run
    return func()
  File "/home/jvlek/dev/lib/happybase/tests/test_api.py", line 76, in teardown_module
    connection.delete_table(TEST_TABLE_NAME, disable=True)
  File "/home/jvlek/dev/lib/happybase/happybase/connection.py", line 320, in delete_table
    if disable and self.is_table_enabled(name):
  File "/home/jvlek/dev/lib/happybase/happybase/connection.py", line 351, in is_table_enabled
    return self.client.isTableEnabled(name)
  File "/home/jvlek/dev/lib/happybase/happybase/hbase/Hbase.py", line 727, in isTableEnabled
    return self.recv_isTableEnabled()
  File "/home/jvlek/dev/lib/happybase/happybase/hbase/Hbase.py", line 744, in recv_isTableEnabled
    raise x
thrift.Thrift.TApplicationException: Negative length: -2147418111

The tear down failure also caused an NPE at the server's side:

2015-04-21 17:10:21,018 ERROR [thrift-worker-0] thrift.ProcessFunction: Internal error processing isTableEnabled
java.lang.NullPointerException
        at org.apache.hadoop.hbase.util.Bytes.getBytes(Bytes.java:968)
        at org.apache.hadoop.hbase.thrift.ThriftServerRunner$HBaseHandler.isTableEnabled(ThriftServerRunner.java:675)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:606)
        at org.apache.hadoop.hbase.thrift.HbaseHandlerMetricsProxy.invoke(HbaseHandlerMetricsProxy.java:67)
        at com.sun.proxy.$Proxy6.isTableEnabled(Unknown Source)
        at org.apache.hadoop.hbase.thrift.generated.Hbase$Processor$isTableEnabled.getResult(Hbase.java:3697)
        at org.apache.hadoop.hbase.thrift.generated.Hbase$Processor$isTableEnabled.getResult(Hbase.java:3681)
        at org.apache.thrift.ProcessFunction.process(ProcessFunction.java:39)
        at org.apache.thrift.TBaseProcessor.process(TBaseProcessor.java:39)
        at org.apache.hadoop.hbase.thrift.TBoundedThreadPoolServer$ClientConnnection.run(TBoundedThreadPoolServer.java:289)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
        at java.lang.Thread.run(Thread.java:745)

I will look into the tearDown() exception now.

[wbolster]

This code you mentioned handles byte strings on Python 2:

## Thrift 0.9.2: lib/py/src/protocol/TBinaryProtocol.py ## 
def writeString(self, str):
  self.writeI32(len(str))
  self.trans.write(str)

The updated code you mentioned is plain wrong if it is still intended to handle byte strings:

## Thrift 1.0.0-dev: lib/py/src/protocol/TBinaryProtocol.py ## 
def writeString(self, str):
  encoded = bytearray(str, 'utf-8')
  self.writeI32(len(encoded))
  self.trans.write(encoded)

This code will handle unicode text only, but I think writeString() is supposed to write byte strings. Additionally, this code (ab)uses a bytearray to encode a unicode string into bytes, while a simple text.encode('utf-8') would have sufficed.

Anyway, the problem is the buggy Thrift code: it needs to make up its mind about unicode text versus bytes. It should handle byte strings.

[jeroenvlek]

I see now that it is not in Thrift's master, but it was introduced by one of the patches for Python 3 support. I will notify that thread.

[jeroenvlek]

Traceback (most recent call last):
  File "/home/jvlek/.virtualenvs/happybase/lib/python3.4/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/jvlek/dev/lib/happybase/tests/test_api.py", line 360, in test_scan
    list(table.scan(batch_size=None))
  File "/home/jvlek/dev/lib/happybase/happybase/table.py", line 294, in scan
    if batch_size < 1:
TypeError: unorderable types: NoneType() < int()

In Python 3, comparison with None raises a TypeError. Which in the context of table.scan() makes sense, because batch_size should be an integer >= 1. The TypeError conveys more precise information, without having to change any code. So I suggest updating test_scan() as follows:

## tests_api.test_scan() ##
with assert_raises(TypeError):
  list(table.scan(batch_size=None))

with assert_raises(ValueError):
  list(table.scan(batch_size=0))

[jeroenvlek]

Traceback (most recent call last):
  File "/home/jvlek/.virtualenvs/happybase/lib/python3.4/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/jvlek/dev/lib/happybase/tests/test_util.py", line 34, in check
    s = s_hex.decode('hex')
AttributeError: 'str' object has no attribute 'decode'

This one seems like an architectural decision. Changing the decode() call for codec.decode(s_hex, 'hex'), led me to a "TypeError: ord() expected string of length 1, but int found".

util.str_increment() says its intent is to increment and truncate a byte string, so it would be enough to remove the call to ord(), since s[i] would already result in an integer. However, then other tests start to fail, because, for example, the row_prefix for table.scan is not a byte string.

Now, I guess you don't want to convert all strings to byte strings, yet the str_increment is written with byte strings in mind, and assumes a hexadecimal space. However, HBase's row keys are UTF-8 encoded, so I reckon there's no need to wrap around after '\xFF'.

Please find my proposed fix below. All the tests pass, but I don't know if there are any side-effects I'm overlooking, or whether you have a completely different direction in mind.

## happybase/util.py ##
MAX_UNICODE = chr(1114111)

def str_increment(s):
    """Increment and truncate a string (for sorting purposes)

    This functions returns the shortest string that sorts after the given
    string when compared using regular string comparison semantics.

    This function increments the last character that is smaller than MAX_UNICODE, and
    drops everything after it. If the string only contains MAX_UNICODE characters,
    `None` is returned.
    """
    for i in range(len(s) - 1, -1, -1):
        if s[i] != MAX_UNICODE:
            return s[:i] + chr(ord(s[i]) + 1)

    return None

## tests/test_util.py ##
def test_str_increment():
    def check(s, expected):
        v = util.str_increment(s)
        assert_equal(expected, v)
        assert_less(s, v)

    test_values = [
        ('00', '01'),
        ('01', '02'),
        ('fe', 'ff'),
        ('1234', '1235'),
        ('12fe', '12ff'),
        ('12ff', '12fg'),
        ('424242' + util.MAX_UNICODE, '424243'),
        ('4242' + 2 * util.MAX_UNICODE, '4243'),
    ]

    assert util.str_increment(util.MAX_UNICODE * 4) is None

    for s, expected in test_values:
        yield check, s, expected

[wbolster]

HBase's row keys are UTF-8 encoded

That's not true. Row keys are byte strings, and a lot of data models depend on that.

[wbolster]

so it would be enough to remove the call to ord(), since s[i] would already result in an integer

That will break on Python 2, since b'foo'[0] will be b'f' on Python 2, and 102 on Python 3. Some compat code is needed here I think. In my Plyvel project I have had the same issue for the same routine.

[jeroenvlek]

That's not true. Row keys are byte strings, and a lot of data models depend on that.

Ah yes, I should have read the follow up comment on this one: http://permalink.gmane.org/gmane.comp.java.hadoop.hbase.user/18006

Although I guess for generating end keys, it still suffices, albeit potentially slightly less efficient (i.e. sending longer end keys than necessary to the server).

That will break on Python 2

Another reason to leave it in (I left the call to ord() in the code).

However, the call to chr() is not backwards compatible, since the equivalent under Python 2 would be unichr(). How did you solve it in your Plyvel project? A quick search on "str_increment" didn't yield anything.

[wbolster]

Few quick follow-up notes:

• No, it must do the right thing on the byte level, not on the UTF-8 encoding level. This code is used to determine the stop key when using a row prefix, so it must be on the byte level.
• unichr() should be avoided; it operates on unicode strings. we're talking bytes here.
• In Plyvel the routine is called bytes_increment(), it uses a bytearray under the hood. (That one has PyPy compat issues after compiling it to C code, but that doesn't apply here.)