[jira] [Comment Edited] (THRIFT-2642) Recursive structs don't work in python

["Eric Conner (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-2642?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16056824#comment-16056824 ] 

Eric Conner edited comment on THRIFT-2642 at 6/21/17 2:03 AM:
--------------------------------------------------------------

Hi everyone,

I finally had some time to take a stab at fixing this.  

I more or less followed the fbthrift approach with a few modifications.  At a high level, the change:

# Moves all thrift_spec definitions to the end of the ttypes or service file.
# Changes the initial declaration of thrift_spec to (<Struct>, None) 
# Uses a method call ``fix_spec`` to fill in the spec to (<Struct>, <Struct>.thrift_spec)

The fbthrift approach is similar except their struct definition uses a list [<Struct>, <Struct>.thrift_spec].  I looked at this route for awhile, but Apache Thrift's fastbinary implementation depends pretty heavily on ``thrift_spec`` being immutable.  There are ref counting assumptions I don't fully understand that led to lots of complications when I tried to change things to use lists (see https://github.com/apache/thrift/blob/19baeefd8c38d62085891d7956349601f79448b3/lib/py/src/ext/protocol.tcc#L341).

Instead, I'm using a modified version of fbthrift's fix_spec script (https://github.com/facebook/fbthrift/blob/3ef868c969464e935b39e336807af1486b2739c3/thrift/lib/py/util/Recursive.py).  The main change I made was to convert each spec to a list, modify the list recursively, and then convert the list back to a tuple.  I'm unsure of the performance hit this adds, but I figured it wouldn't be too bad since it only happens at import time.

Finally, I had *lots* of trouble trying to figure out how to add tests for this to the python test harness.  I wanted to add Recursive.thrift to the generated test code for python and then add a basic test which uses it.  I could not figure out how to add Recursive.thrift to the gen-py makefile.  It seems like the files that need to change are Makefile.am and generate.cmake within thrift/test/py.  When I do that the only way I could get the code generated was by re-running bootstrap.sh, configure, and make which has like a 10 minute turnaround time on my machine.  Surely there is a faster way to do that?  Second, I was really unclear on what all the gen-py-* directories were for.  Can anyone clarify that for me?

Here is my external test script that I'd like to add:
{code:python}
import thrift

from Recursive.ttypes import *

def test_rec_tree():
    print "Tree Test ..",
    children = []
    for idx in xrange(1, 5):
        node = RecTree(idx)
        children.append(node)

    parent = RecTree(item=0, children=children)
    assert len(parent.children) == 4
    for child in parent.children:
        assert isinstance(child, RecTree)
    print "OK"

def _get_linked_list():
    head = cur = RecList(item=0)
    for idx in xrange(1, 5):
        node = RecList(item=idx)
        cur.nextitem = node
        cur = node
    return head

def _collapse_linked_list(head):
    out_list = []
    cur = head
    while cur is not None:
        out_list.append(cur.item)
        cur = cur.nextitem
    return out_list

def test_rec_list():
    print "Linked List Test ..",
    head = _get_linked_list()

    golden_list = [0, 1, 2, 3, 4]
    test_list = _collapse_linked_list(head)

    assert golden_list == test_list
    print "OK"

def test_co_rec():
    print "Co Rec Test ..",

    item1 = CoRec()
    item2 = CoRec2()

    item1.other = item2
    item2.other = item1

    print "OK"

def test_vector():
    print "Vector Test ..",
    mylist = [_get_linked_list(), _get_linked_list()]
    myvec = VectorTest(lister=mylist)

    golden_list = [0, 1, 2, 3, 4]
    for cur_list in myvec.lister:
        out_list = _collapse_linked_list(cur_list)
        assert golden_list == out_list

    print "OK"


if __name__ == "__main__":
    test_rec_tree()
    test_rec_list()
    test_co_rec()
    test_vector()
{code}







was (Author: econner724):
Hi everyone,

I finally had some time to take a stab at fixing this.  

I more or less followed the fbthrift approach with a few modifications.  At a high level, the change:

# Moves all thrift_spec definitions to the end of the ttypes or service file.
# Changes the initial declaration of thrift_spec to (<Struct>, None) 
# Uses a method call ``fix_spec`` to fill in the spec to (<Struct>, <Struct>.thrift_spec)

The fbthrift approach is similar except their struct definition uses a list [<Struct>, <Struct>.thrift_spec].  I looked at this route for awhile, but Apache Thrift's fastbinary implementation depends pretty heavily on ``thrift_spec`` being immutable.  There are ref counting assumptions I don't fully understand that led to lots of complications when I tried to change things to use lists (see https://github.com/apache/thrift/blob/19baeefd8c38d62085891d7956349601f79448b3/lib/py/src/ext/protocol.tcc#L341).

Instead, I'm using a modified version of fbthrift's fix_spec script (https://github.com/facebook/fbthrift/blob/3ef868c969464e935b39e336807af1486b2739c3/thrift/lib/py/util/Recursive.py).  The main change I made was to convert each spec to a list, modify the list recursively, and then convert the list back to a tuple.  I'm unsure of the performance hit this adds, but I figured it wouldn't be too bad since it only happens at import time.

 
 







> Recursive structs don't work in python
> --------------------------------------
>
>                 Key: THRIFT-2642
>                 URL: https://issues.apache.org/jira/browse/THRIFT-2642
>             Project: Thrift
>          Issue Type: Bug
>          Components: Python - Compiler, Python - Library
>    Affects Versions: 0.9.2
>            Reporter: Igor Kostenko
>
> Recursive structs in 0.9.2 work fine in c++ & c#, but not in python, because generated code trying to use objects which not constructed yet.
> Struct:
> {code}
> struct Recursive {
> 1: list<Recursive> Children
> }
> {code}
> Python code:
> {code}
> class Recursive:
>   thrift_spec = (
>     None, # 0
>     (1, TType.LIST, 'Children', (TType.STRUCT,(Recursive, Recursive.thrift_spec)), None, ), # 1
>   )
> {code}
> Error message:
> {code}
> Traceback (most recent call last):
>   File "ttypes.py", line 20, in <module>
>     class Recursive:
>   File "ttypes.py", line 28, in Recursive
>     (1, TType.LIST, 'Children', (TType.STRUCT,(Recursive, Recursive.thrift_spec)), None, ), # 1
> NameError: name 'Recursive' is not defined
> {code}



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)