You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by econner <gi...@git.apache.org> on 2017/06/21 01:55:54 UTC
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
GitHub user econner opened a pull request:
https://github.com/apache/thrift/pull/1293
THRIFT-2642 Recursive structs don't work in python
See https://issues.apache.org/jira/browse/THRIFT-2642.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/econner/thrift master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1293.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1293
----
commit d62798144b68353c8def2e80388b9ae5e368e9fb
Author: Eric Conner <er...@pinterest.com>
Date: 2017-06-21T01:34:12Z
THRIFT-2642 Recursive structs don't work in python
See https://issues.apache.org/jira/browse/THRIFT-2642.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1293: THRIFT-2642 Recursive structs don't work in python
Posted by econner <gi...@git.apache.org>.
Github user econner commented on the issue:
https://github.com/apache/thrift/pull/1293
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by juliengreard <gi...@git.apache.org>.
Github user juliengreard commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1293#discussion_r125759228
--- Diff: lib/py/src/TRecursive.py ---
@@ -0,0 +1,63 @@
+# MODIFIED June 20, 2017, Eric Conner
+#
+#
+# Original source copyright 2014-present Facebook, Inc.
+#
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+from __future__ import unicode_literals
+
+from thrift.Thrift import TType
+
+
+def fix_spec(all_structs):
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by econner <gi...@git.apache.org>.
Github user econner commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1293#discussion_r125777018
--- Diff: test/py/SerializationTest.py ---
@@ -285,6 +290,67 @@ def testIntegerLimits(self):
for value in bad_values:
self.assertRaises(Exception, self._serialize, value)
+ def testRecTree(self):
+ """Ensure recursive tree node can be created."""
+ children = []
+ for idx in range(1, 5):
--- End diff --
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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1293#discussion_r125749743
--- Diff: lib/py/src/TRecursive.py ---
@@ -0,0 +1,63 @@
+# MODIFIED June 20, 2017, Eric Conner
+#
+#
+# Original source copyright 2014-present Facebook, Inc.
+#
--- End diff --
Where does line 1 to 5 come from? That's not part of the standard ASF license header.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by econner <gi...@git.apache.org>.
Github user econner closed the pull request at:
https://github.com/apache/thrift/pull/1293
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by juliengreard <gi...@git.apache.org>.
Github user juliengreard commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1293#discussion_r125757042
--- Diff: compiler/cpp/src/thrift/generate/t_py_generator.cc ---
@@ -610,11 +623,22 @@ string t_py_generator::render_const_value(t_type* type, t_const_value* value) {
return out.str();
}
+/**
+ * Generates the "forward declarations" for python structs.
+ * These are actually full class definitions so that calls to generate_struct
+ * 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) {
+ generate_py_struct(tstruct, tstruct->is_xception());
+}
+
/**
* Generates a python struct
*/
void t_py_generator::generate_struct(t_struct* tstruct) {
- generate_py_struct(tstruct, false);
+ //generate_py_struct(tstruct, false);
--- End diff --
bad idea to leave commented code - if you really want to leave it there, leave also a comment explaining why
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by econner <gi...@git.apache.org>.
Github user econner commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1293#discussion_r125751862
--- Diff: lib/py/src/TRecursive.py ---
@@ -0,0 +1,63 @@
+# MODIFIED June 20, 2017, Eric Conner
+#
+#
+# Original source copyright 2014-present Facebook, Inc.
+#
--- End diff --
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...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by econner <gi...@git.apache.org>.
Github user econner commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1293#discussion_r125776924
--- Diff: lib/py/src/ext/types.cpp ---
@@ -20,6 +20,8 @@
#include "ext/types.h"
#include "ext/protocol.h"
+#include <iostream>
--- End diff --
Mm I believe this was a holdover from testing & using print statements. It should be safe to remove. Thanks for catching.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by econner <gi...@git.apache.org>.
Github user econner commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1293#discussion_r125776706
--- Diff: compiler/cpp/src/thrift/generate/t_py_generator.cc ---
@@ -610,11 +623,22 @@ string t_py_generator::render_const_value(t_type* type, t_const_value* value) {
return out.str();
}
+/**
+ * Generates the "forward declarations" for python structs.
+ * These are actually full class definitions so that calls to generate_struct
+ * 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) {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by juliengreard <gi...@git.apache.org>.
Github user juliengreard commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1293#discussion_r125761010
--- Diff: test/py/SerializationTest.py ---
@@ -285,6 +290,67 @@ def testIntegerLimits(self):
for value in bad_values:
self.assertRaises(Exception, self._serialize, value)
+ def testRecTree(self):
+ """Ensure recursive tree node can be created."""
+ children = []
+ for idx in range(1, 5):
--- End diff --
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)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by juliengreard <gi...@git.apache.org>.
Github user juliengreard commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1293#discussion_r125757570
--- Diff: compiler/cpp/src/thrift/generate/t_py_generator.cc ---
@@ -634,6 +659,49 @@ void t_py_generator::generate_py_struct(t_struct* tstruct, bool is_exception) {
generate_py_struct_definition(f_types_, tstruct, is_exception);
}
+
+/**
+ * Generate the thrift_spec for a struct
+ * e.g. (4, TType.LIST, 'struct_list', (TType.STRUCT, (RandomStuff, None), False), None, ), # 4
+ */
+void t_py_generator::generate_py_thrift_spec(ofstream& out,
+ t_struct* tstruct,
+ bool /*is_exception*/) {
+ const vector<t_field*>& sorted_members = tstruct->get_sorted_members();
+ vector<t_field*>::const_iterator m_iter;
+
+ // Add struct definition to list so thrift_spec can be fixed for recursive structures.
+ indent(out) << "all_structs.append(" << tstruct->get_name() << ")" << endl;
+
+ if (sorted_members.empty() || (sorted_members[0]->get_key() >= 0)) {
+ indent(out) << tstruct->get_name() << ".thrift_spec = (" << endl;
+ indent_up();
+
+ int sorted_keys_pos = 0;
+ for (m_iter = sorted_members.begin(); m_iter != sorted_members.end(); ++m_iter) {
+
+ for (; sorted_keys_pos != (*m_iter)->get_key(); sorted_keys_pos++) {
+ indent(out) << "None, # " << sorted_keys_pos << endl;
+ }
+
+ indent(out) << "(" << (*m_iter)->get_key() << ", " << type_to_enum((*m_iter)->get_type())
--- End diff --
a comment showing an example of the python code generated from this peace of code would be a plus
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1293
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by juliengreard <gi...@git.apache.org>.
Github user juliengreard commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1293#discussion_r125759386
--- Diff: lib/py/src/ext/types.cpp ---
@@ -20,6 +20,8 @@
#include "ext/types.h"
#include "ext/protocol.h"
+#include <iostream>
--- End diff --
I usually include built-in lib first
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by juliengreard <gi...@git.apache.org>.
Github user juliengreard commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1293#discussion_r125756848
--- Diff: compiler/cpp/src/thrift/generate/t_py_generator.cc ---
@@ -610,11 +623,22 @@ string t_py_generator::render_const_value(t_type* type, t_const_value* value) {
return out.str();
}
+/**
+ * Generates the "forward declarations" for python structs.
+ * These are actually full class definitions so that calls to generate_struct
+ * 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) {
--- End diff --
I dont't see where this function is called. Maybe I don't have the full diff in front of me
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1293: THRIFT-2642 Recursive structs don't work in python
Posted by econner <gi...@git.apache.org>.
Github user econner commented on the issue:
https://github.com/apache/thrift/pull/1293
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1293: THRIFT-2642 Recursive structs don't work in pytho...
Posted by econner <gi...@git.apache.org>.
GitHub user econner reopened a pull request:
https://github.com/apache/thrift/pull/1293
THRIFT-2642 Recursive structs don't work in python
See https://issues.apache.org/jira/browse/THRIFT-2642.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/econner/thrift master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1293.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1293
----
commit d62798144b68353c8def2e80388b9ae5e368e9fb
Author: Eric Conner <er...@pinterest.com>
Date: 2017-06-21T01:34:12Z
THRIFT-2642 Recursive structs don't work in python
See https://issues.apache.org/jira/browse/THRIFT-2642.
commit b178f8109e6a0f94005ed65db506e2b14ac41a9d
Author: Eric Conner <er...@pinterest.com>
Date: 2017-06-21T14:52:44Z
Fix build issues.
commit 7eb8e319385185904161185602f902bee0ff4544
Author: Eric Conner <er...@pinterest.com>
Date: 2017-06-21T17:00:57Z
Convert spec for TStruct to list in python.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---