New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

THRIFT-2642 Recursive structs don't work in python #1293

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@econner

This comment has been minimized.

Copy link

econner commented Jun 21, 2017

As written this will not work. The spec for a Struct needs to be a reference type such as a list for recursion to work correctly so there is more work to be done here.

@econner econner closed this Jun 21, 2017

@econner econner reopened this Jun 21, 2017

@econner

This comment has been minimized.

Copy link

econner commented Jun 21, 2017

Ok, I've updated the PR to use a list (reference type) instead of a tuple. I also added some serialization tests for recursive structures. Hopefully everything works now.

@econner econner force-pushed the econner:master branch from bdcf692 to 2372bb6 Jun 27, 2017

#
#
# Original source copyright 2014-present Facebook, Inc.
#

This comment has been minimized.

@Jens-G

Jens-G Jul 5, 2017

Member

Where does line 1 to 5 come from? That's not part of the standard ASF license header.

This comment has been minimized.

@econner

econner Jul 5, 2017

Yea, I'm not very experienced with the whole issue of licensing. I mainly put these lines because of the fbthrift license and I used this file more or less verbatim from fbthrift. (https://github.com/facebook/fbthrift/blob/master/LICENSE#L97) (https://github.com/facebook/fbthrift/blob/master/thrift/lib/py/util/Recursive.py)

  (b) You must cause any modified files to carry prominent notices
      stating that You changed the files; and

  (c) You must retain, in the Source form of any Derivative Works
      that You distribute, all copyright, patent, trademark, and
      attribution notices from the Source form of the Work,
      excluding those notices that do not pertain to any part of
      the Derivative Works; and

But perhaps it is fine to use verbatim since fbthrift is itself a branch of thrift? Not really sure what the protocol here is...

* can add the thrift_spec field. This is needed so that all thrift_spec
* definitions are grouped at the end of the file to enable co-recursive structs.
*/
void t_py_generator::generate_forward_declaration(t_struct* tstruct) {

This comment has been minimized.

@juliengreard

juliengreard Jul 5, 2017

I dont't see where this function is called. Maybe I don't have the full diff in front of me

This comment has been minimized.

@econner

econner Jul 5, 2017

This is an interface method that's called by the thrift compiler. One attribute of this change is to move most of the code that generates python code into generate_forward_declaration and then output the thrift_spec via the generate_struct method.

/**
* Generates a python struct
*/
void t_py_generator::generate_struct(t_struct* tstruct) {
generate_py_struct(tstruct, false);
//generate_py_struct(tstruct, false);

This comment has been minimized.

@juliengreard

juliengreard Jul 5, 2017

bad idea to leave commented code - if you really want to leave it there, leave also a comment explaining why

indent(out) << "None, # " << sorted_keys_pos << endl;
}

indent(out) << "(" << (*m_iter)->get_key() << ", " << type_to_enum((*m_iter)->get_type())

This comment has been minimized.

@juliengreard

juliengreard Jul 5, 2017

a comment showing an example of the python code generated from this peace of code would be a plus

from thrift.Thrift import TType


def fix_spec(all_structs):

This comment has been minimized.

@juliengreard

juliengreard Jul 5, 2017

Well I'm sorry but I dont understand this file. Maybe explaining what the functions are doing would help. You could also use named variables instead of using directly 'magic' numbers (like const static int name = 1). But I may be wrong, I'm a complete stranger to the source code of thrift, so maybe it's the way to do it

@@ -20,6 +20,8 @@
#include "ext/types.h"
#include "ext/protocol.h"

#include <iostream>

This comment has been minimized.

@juliengreard

juliengreard Jul 5, 2017

I usually include built-in lib first

This comment has been minimized.

@econner

econner Jul 5, 2017

Mm I believe this was a holdover from testing & using print statements. It should be safe to remove. Thanks for catching.

def testRecTree(self):
"""Ensure recursive tree node can be created."""
children = []
for idx in range(1, 5):

This comment has been minimized.

@juliengreard

juliengreard Jul 5, 2017

where do the "5" comes from ? Is it a limit for the implementation (you can't have recursive struct with more than 5 recursions? if so, it should be highlighted (maybe it is already, but I didn't see it). What happens if I have a struct with more than 5 recursions in it (i.e. myObject.leaf.leaf.leaf.leaf.leaf.leaf.leaf.leaf is not None)?

This comment has been minimized.

@econner

econner Jul 5, 2017

This is just a test to ensure you can make a tree with some children. There is no limit on the number of recursions (I guess besides the maximum allowed by python).

@econner econner force-pushed the econner:master branch from 2372bb6 to fddf2f0 Jul 6, 2017

Eric Conner

@asfgit asfgit closed this in c34653f Jul 6, 2017

jeking3 added a commit to jeking3/thrift that referenced this pull request Aug 5, 2017

THRIFT-2642 Recursive structs don't work in python
Client: Python
Patch: Eric Conner <eric@pinterest.com>

This closes apache#1293

jeking3 added a commit to jeking3/thrift that referenced this pull request Nov 30, 2017

THRIFT-2642 Recursive structs don't work in python
Client: Python
Patch: Eric Conner <eric@pinterest.com>

This closes apache#1293
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment