You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Bryan Duxbury (JIRA)" <ji...@apache.org> on 2008/09/19 19:44:44 UTC
[jira] Created: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Generated Thrift structs should have a deep-copy constructor
------------------------------------------------------------
Key: THRIFT-138
URL: https://issues.apache.org/jira/browse/THRIFT-138
Project: Thrift
Issue Type: New Feature
Components: Compiler (Java)
Reporter: Bryan Duxbury
Priority: Minor
It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12649465#action_12649465 ]
Bryan Duxbury commented on THRIFT-138:
--------------------------------------
David, can you look at this issue? It looks like Piotr has posted an updated patch that should satisfy your requests.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Piotr Kozikowski
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7-test.patch, thrift-138-v7.patch, thrift-138-v8.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Piotr Kozikowski (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12651538#action_12651538 ]
Piotr Kozikowski commented on THRIFT-138:
-----------------------------------------
Please go ahead and commit it.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Piotr Kozikowski
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7-test.patch, thrift-138-v7.patch, thrift-138-v8.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-138:
---------------------------------
Attachment: thrift-138-v3.patch
I think this patch fixes the problems you highlighted.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12640261#action_12640261 ]
Bryan Duxbury commented on THRIFT-138:
--------------------------------------
I don't really understand what the problem with the compiler is. Why is "t_java_generator::" unnecessary in the declaration of generate_deep_copy_non_container? I only mimicked the way other methods were defined.
If you are having specific troubles, can you post the errors you are seeing?
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Mark Slee (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12636441#action_12636441 ]
Mark Slee commented on THRIFT-138:
----------------------------------
Actually, scratch that latter suggestion. putAll() and addAll() only make shallow copies. Bummer.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12638398#action_12638398 ]
David Reiss commented on THRIFT-138:
------------------------------------
I don't know. I don't really like the idea of having a clone method that returns a one-level-deep copy of the structure. Can we put it to a vote?
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12640636#action_12640636 ]
David Reiss commented on THRIFT-138:
------------------------------------
1/ By checking __isset before copying a container, we're implicitly expecting it to be properly maintained, which I think we should avoid. If the type is primitive, there is no harm to just copying it unconditionally. If it is nonprimitive, then I think checking for null is the best choice in all circumstances.
2/ You have a "++m_iter" that should be moved into the for-loop header.
3/ Is the change to TestClient supposed to be part of this diff?
4/ There is a possibility that your temporary local variables could collide with field names. Can you prefix all member variable references with "this." to avoid problems in this case?
5/ I think we should have a thorough test case for this code. For example, I'm pretty sure there is a null pointer exception lurking.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
David Reiss resolved THRIFT-138.
--------------------------------
Resolution: Fixed
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Piotr Kozikowski
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7-test.patch, thrift-138-v7.patch, thrift-138-v8.patch, thrift-138-v9.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Posted by "Piotr Kozikowski (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Piotr Kozikowski updated THRIFT-138:
------------------------------------
Attachment: thrift-138-v9.patch
To fix this seemed very straightforward, but actually it was quite tricky. Here is a patch. It's a bit of a hack but I tested it very thoroughly. I added a simple test case to DeepCopyTest.java. You can check that without the changes it doesn't pass.
I also did a more specific test locally as follows:
thrift file:
struct SomeStruct {
1: i32 num,
2: string name,
3: binary bin,
4: map<binary,binary> mapvar
5: list<binary> listvar
}
java file:
SomeStruct s = new SomeStruct();
s.setBin("hello".getBytes());
s.setMapvar(new HashMap<byte[], byte[]>());
s.getMapvar().put("key".getBytes(), "value".getBytes());
s.setListvar(new LinkedList<byte[]>());
s.getListvar().add("element".getBytes());
SomeStruct s2 = new SomeStruct(s);
// Basic case
s.getBin()[0]++;
if (s.equals(s2))
System.out.println("Error!");
s.getBin()[0]--;
// Map key
s.getMapvar().keySet().iterator().next()[0]++;
if (s.equals(s2))
System.out.println("Error!");
s.getMapvar().keySet().iterator().next()[0]--;
// Map value
s.getMapvar().values().iterator().next()[0]++;
if (s.equals(s2))
System.out.println("Error!");
s.getMapvar().values().iterator().next()[0]--;
// List
s.getListvar().get(0)[0]++;
if (s.equals(s2))
System.out.println("Error!");
Without the patch, I got all four errors, and with it none of them. I think this pretty much covers all possible cases.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Piotr Kozikowski
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7-test.patch, thrift-138-v7.patch, thrift-138-v8.patch, thrift-138-v9.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12636440#action_12636440 ]
Bryan Duxbury commented on THRIFT-138:
--------------------------------------
I'll look at declare_fields more closely.
We can't use putAll and addAll because we need to make a deep copy of all the elements.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12640780#action_12640780 ]
Bryan Duxbury commented on THRIFT-138:
--------------------------------------
1/
I've had trouble with .equals() using the default generator that go away when using the beans generator. The toString() output makes the objects look exactly the same, so there must be some other bug related to __isset.
The fact that you have to manually maintain the __isset fields if you want to get properly sparse serialization is also kind of problematic (THRIFT-33), especially since this is a key advantage of Thrift.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12640283#action_12640283 ]
David Reiss commented on THRIFT-138:
------------------------------------
Sorry to be unclear. The ones in the method definitions are required. The ones at the top (in the class definition) are causing the error.
{noformat}
ccache g++ -DHAVE_CONFIG_H -I. -I../.. -Wall -I./src -I/usr/include -g -O2 -MT thrift-t_java_generator.o -MD -MP -MF .deps/thrift-t_java_generator.Tpo -c -o thrift-t_java_generator.o `test -f 'src/generate/t_java_generator.cc' || echo './'`src/generate/t_java_generator.cc
src/generate/t_java_generator.cc:157: error: extra qualification 't_java_generator::' on member 'generate_deep_copy_non_container'
dreiss@dr-mbp:t138:thrift:2$ g++ --version
g++ (GCC) 4.2.3 (Ubuntu 4.2.3-2ubuntu7)
{noformat}
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12649800#action_12649800 ]
Bryan Duxbury commented on THRIFT-138:
--------------------------------------
I think it looks good. Go ahead and commit it.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Piotr Kozikowski
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7-test.patch, thrift-138-v7.patch, thrift-138-v8.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12638613#action_12638613 ]
Bryan Duxbury commented on THRIFT-138:
--------------------------------------
I guess it's possible to create a recursive method in the generator that keeps on unwrapping nested collections until it actually hits a copyable structure. If I find a few minutes I will take a crack at this.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-138:
---------------------------------
Attachment: thrift-138-v7.patch
OK, I think that this solves that problem.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12652234#action_12652234 ]
David Reiss commented on THRIFT-138:
------------------------------------
Hey guys. I just realized that this doesn't work properly for binary fields. Any chance you could implement that?
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Piotr Kozikowski
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7-test.patch, thrift-138-v7.patch, thrift-138-v8.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12640255#action_12640255 ]
David Reiss commented on THRIFT-138:
------------------------------------
This seems like the right approach. Unfortunately, the generated code (for SmallTest.thrift, for example) doesn't compile. Actually, the compiler doesn't even compile (for me) because of the unnecessary "t_java_generator::" in "void t_java_generator::generate_deep_copy_non_container".
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12640797#action_12640797 ]
David Reiss commented on THRIFT-138:
------------------------------------
How about any structure containing a map?
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Nathan Marz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12636437#action_12636437 ]
Nathan Marz commented on THRIFT-138:
------------------------------------
I like it. What if we added a "deepCopy" method to each object as well, so that if you have a TBase object but don't know it's specific type you can still do deep copies?
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-138:
---------------------------------
Attachment: thrift-138-v2.patch
This version uses type_name more effectively, per Mark. I've also added a clone() method and implemented the Cloneable interface so that you can deep copy and object when you don't know its type, per Nathan.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12640661#action_12640661 ]
David Reiss commented on THRIFT-138:
------------------------------------
1/ What is unreliable? The original use case for __isset was to determine which fields were present after reading a structure off the wire. It was never meant to be an authoritative representation of the current state of the structure.
3/ Not necessary. I'll just commit it as a separate rev.
5/ I meant completely independent from the contents of the container. I think that if you build and run a test case for this code, it will be clear what the problem and solution are.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-138:
---------------------------------
Attachment: thrift-138-v6.patch
This patch includes a test for deep copying a struct with a map and a list, which did turn up an NPE.
I'm having trouble concocting a rational set of field names that conflict with generated local variables, so I've held off solving that problem. I mean, if you have a map called myMap and another field called myMap_element, I guess that might possibly get you, but it seems odd.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12638391#action_12638391 ]
Bryan Duxbury commented on THRIFT-138:
--------------------------------------
You're right, that very likely does not make a deep copy of the nested map. Without getting into some crazy scenario where we have to potentially recursively manage the nesting, I don't think there's any way to resolve this. I'm thinking that we should just not worry about it though. I know that we support nested maps and such, but I think it will cover a minority of cases. We can open another issue with lower priority to track this problem and maybe resolve it in the future.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Posted by "Piotr Kozikowski (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Piotr Kozikowski updated THRIFT-138:
------------------------------------
Attachment: thrift-138-v8.patch
David,
Here is an updated patch that gets rid of the temporary variables. As for the other recommendations I'm not sure I get the point. When you say "make the code even shorter and simpler" are you referring to the compiler's cpp code or the generated java code?
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7-test.patch, thrift-138-v7.patch, thrift-138-v8.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12637727#action_12637727 ]
David Reiss commented on THRIFT-138:
------------------------------------
In the map case, this line needs an endl.
bq: out << type_name(type, false, true) << "();";
In the set list case, I think this line
bq: out << type_name(type, true, true) << "();" << endl;
should be using (type, false, true) because it is not in a container. I doesn't make a difference, but it makes it consistent with the map case.
This version doesn't appear to handle nested containers properly. The output for SmallTest.thrift contains...
{noformat}
for (Map.Entry<Integer, Map<Integer,Integer>> entry : other.complexer.entrySet()) {
complexer.put(new Integer(entry.getKey()), new Map<Integer,Integer>(entry.getValue()));
}
{noformat}
I'm pretty sure new Map<K,V> won't compile because Map is an interface.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Posted by "Piotr Kozikowski (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Piotr Kozikowski updated THRIFT-138:
------------------------------------
Attachment: thrift-138-v7-test.patch
Hi David,
I'm working with Bryan and he delegated to me some of the stuff related to Thrift. Here is a test that follows your recommendations. It passes. Given the complexity of the object being copied it would be surprising to find more bugs in the compiler's code.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7-test.patch, thrift-138-v7.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-138:
---------------------------------
Attachment: thrift-138-v4.patch
This patch is still a little rough, but I think it gets most of the problem solved. Take a look.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury reassigned THRIFT-138:
------------------------------------
Assignee: Bryan Duxbury
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury reassigned THRIFT-138:
------------------------------------
Assignee: Piotr Kozikowski (was: Bryan Duxbury)
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Piotr Kozikowski
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7-test.patch, thrift-138-v7.patch, thrift-138-v8.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12652277#action_12652277 ]
Bryan Duxbury commented on THRIFT-138:
--------------------------------------
Hm. I'm guessing you mean it doesn't perform an actual deep copy on the byte[]s? That should be a relatively easy fix with System.arrayCopy().
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Piotr Kozikowski
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7-test.patch, thrift-138-v7.patch, thrift-138-v8.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-138:
---------------------------------
Patch Info: [Patch Available]
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12640781#action_12640781 ]
Bryan Duxbury commented on THRIFT-138:
--------------------------------------
Can you give me some sense of what kind of scenario would uncover a null pointer exception?
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-138:
---------------------------------
Attachment: thrift-138.patch
Here's a patch that implements a deep-copy constructor.
I created a small test with one of our structs that has several fields, but only one string and one enum actually set. The serialization method was two orders of magnitude slower than the copy constructor.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12638387#action_12638387 ]
David Reiss commented on THRIFT-138:
------------------------------------
I'm not sure whether this implementation produces a deep copy for nested structures. For example, this code is in Hello.java
{noformat}
for (Map.Entry<Integer, HashMap<Integer,Integer>> entry : other.complexer.entrySet()) {
complexer.put(new Integer(entry.getKey()), new HashMap<Integer,Integer>(entry.getValue()));
}
{noformat}
I suspect that "new HashMap<Integer,Integer>(entry.getValue())" only does a shallow copy of the inner map, based on what the .clone() method for HashMap does. It's seems pretty shameful that there is no standard way to do a deep copy in Java. I'm not really sure how to resolve this.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12640647#action_12640647 ]
Bryan Duxbury commented on THRIFT-138:
--------------------------------------
1/ Understood, though it bothers me that we're not willing to trust the __isset fields. This goes back to the reason why Rapleaf switched to the java:beans generator in the first place - lots of stuff is unreliable with the default generator.
2/ Will move.
3/ Yes, but it doesn't really belong to this issue. It rightly belongs to the issue that removed the optional framing from TFramedTransport, but is required to run the tests. I can create a separate issue, if you'd like.
4/ Makes sense, will do so.
5/ There is probably potential for a null pointer in there. It should be trivial to resolve though, so I'll make some changes and submit a new patch. We do have a separate issue open somewhere about not allowing null elements in our collection, and if we could figure out a way to fix that instead, we'd solve this issue for free.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "Mark Slee (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12636439#action_12636439 ]
Mark Slee commented on THRIFT-138:
----------------------------------
You've reimplemented a lot of the type_name() method in here in the initialization of the containers. You can reuse code to declare the new nested types. Take a look at how declare_field() does this. All you should need to add is the "new" and the "()" parts -- HashSet/HashMap etc. shoudn't show up in the patch.
Functionality-wise, this looks good. Just revise it to use type_name().
Also, I'd recommend using the putAll() and addAll() methods of the Java Collection interfaces, rather than building your own iteration loops to copy each container element individually. The *All() variations might be optimized for better performance on bulk copies/inserts.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12649785#action_12649785 ]
David Reiss commented on THRIFT-138:
------------------------------------
This is the version that I'm thinking about committing: http://gitweb.thrift-rpc.org/?p=thrift.git;a=treediff;h=3e78c4d;hp=d819e76
Broken down into smaller changes: http://gitweb.thrift-rpc.org/?p=thrift.git;a=log;h=refs/heads/pri/dreiss/t138;hb=HEAD
I meant that I thought it would be possible to simplify the C++ code. I coded it up, but it gets into a few problems with autoboxing/unboxing and creates a few extra local variables that are not optimized out by my compiler (though they hopefully would be by the JVM). I think it still might be possible to unify the two generate_deep_copy functions, but I'm no longer convinced that it would be much simpler. If you are okay with this version, I can check it in.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Piotr Kozikowski
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7-test.patch, thrift-138-v7.patch, thrift-138-v8.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12641544#action_12641544 ]
David Reiss commented on THRIFT-138:
------------------------------------
It's definitely odd, but it seems like the fix is super easy, right? I think it is worth trying to solve because the behavior is so insidious.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-138) Generated Thrift structs should have
a deep-copy constructor
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12642066#action_12642066 ]
David Reiss commented on THRIFT-138:
------------------------------------
I think it makes sense to have a more thorough test case for something as tricky as this. The JSON test in Java builds a pretty complex object with nested containers. Can you use that (or something similar) to test the copy constructor? I think it also makes sense to use serialization to generate a known-good copy of the original to make sure that the source is not accidentally modified during the copy (for example, if two parameters in the generator were backwards and it was actually copying the destination into the source).
Also, to test that it is actually doing a deep copy, I think it makes sense to modify some deep-down object in the copy to make sure it is no longer equal to the original. (I wouldn't modify the original because it has internal shared structure.)
Treating typedefs as base types is a bug. Call get_true_type on every type that comes your way to avoid typedef issues.
In the "is_container" branch in the constructor generator, I think you need to use "this.". Also, why bother with temporaries? Why not just generate_deep_copy_constructor straight from outher.field_name to this.field_name?
I think that the code would be simpler if generate_deep_copy_non_container were converted to generate a full assignment like generate_deep_copy_constructor. I only see one place where you aren't doing a normal assignment around it, and I think even that case might be made simpler because you could share the temporary variable with the container case. If the two generate_deep_copy functions were merged into one that incorporated the is_container check, it might make the code even shorter and simpler.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138-v6.patch, thrift-138-v7.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-138) Generated Thrift structs should have a
deep-copy constructor
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-138:
---------------------------------
Attachment: thrift-138-v5.patch
Try this one.
> Generated Thrift structs should have a deep-copy constructor
> ------------------------------------------------------------
>
> Key: THRIFT-138
> URL: https://issues.apache.org/jira/browse/THRIFT-138
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-138-v2.patch, thrift-138-v3.patch, thrift-138-v4.patch, thrift-138-v5.patch, thrift-138.patch
>
>
> It'd be nice to be able to make a copy of a Thrift object by passing it as an argument to the constructor of the same object. Right now, we have to use a serialize/deserialize approach to making a deep object copy, and that seems really inefficient.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.