You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pig.apache.org by "Ashutosh Chauhan (JIRA)" <ji...@apache.org> on 2010/03/11 22:58:27 UTC
[jira] Created: (PIG-1292) Interface Refinements
Interface Refinements
---------------------
Key: PIG-1292
URL: https://issues.apache.org/jira/browse/PIG-1292
Project: Pig
Issue Type: Bug
Affects Versions: 0.7.0
Reporter: Ashutosh Chauhan
Fix For: 0.7.0
A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (PIG-1292) Interface Refinements
Posted by "Dmitriy V. Ryaboy (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12844611#action_12844611 ]
Dmitriy V. Ryaboy commented on PIG-1292:
----------------------------------------
Agreed with Xuefu's comment regarding the interfaces. This really seems like something we can just have the abstract func default to false.
Method name suggestion: how about hasKeyToSplitAffinity()
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (PIG-1292) Interface Refinements
Posted by "Dmitriy V. Ryaboy (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12844640#action_12844640 ]
Dmitriy V. Ryaboy commented on PIG-1292:
----------------------------------------
.. but we have an abstract class that can provide default implementations so that implementers don't have to think about this.
Most of the interfaces introduced in PIG-966 have significant chunks of functionality associated with them. This is just a single method about a particular property of the incoming data.
I can see why you'd be against putting it into LoadFunc, though, as it's very specific. What about ResourceSchema or LoadMetaData?
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Closed: (PIG-1292) Interface Refinements
Posted by "Daniel Dai (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Daniel Dai closed PIG-1292.
---------------------------
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-1292.patch, pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (PIG-1292) Interface Refinements
Posted by "Xuefu Zhang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12844282#action_12844282 ]
Xuefu Zhang commented on PIG-1292:
----------------------------------
A few comments from my personal perspective:
1. WritableComparable<?> getSplitComparable(PigSplit split) should not pass PigSplit instace. Instead, it should only pass the actual split wrapped in PigSplit. On interface level, the type should be just InputSplit.
2. In CollectableLoadFunc interface, void should be the return type. If a LoadFunc implementation implements the interface, it means it's capable to support what is asking for. There is no point to implement an interface and later to say it's not capably to do what the interface is asking.
3. ensureAllKeyInstancesInSameSplit() method might need a better name. However, comparing to another two issues, this is minor.
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (PIG-1292) Interface Refinements
Posted by "Ashutosh Chauhan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12844301#action_12844301 ]
Ashutosh Chauhan commented on PIG-1292:
---------------------------------------
Thanks for review, Xuefu.
1. Thats a valid point. Where possible we want loadfunc implementers to deal with Hadoop concepts and not with Pig concepts.
2. So, lets assume there is a loader which is capable of implementing this interface but only if underlying data is sorted (information which is available to loader only at run-time). Now this loader will implement this interface, indicating to Pig it is capable of doing it. But just because it is capable of doing it, doesn't necessarily imply it will do it (possibly because of performance reasons). Then, when Pig calls the method of interface, it is communicating to loader that it wants data in particular fashion, no matter what. Inside this method, loader may come to know about some metadata (like data is not sorted, possibly by reading its schema or contacting some metadata repo) and decides that it cant honor the contract because of information which is available to it only at run time. Then, loader may return false for the method. Pig may then choose to rewrite the query and still carry-on the execution. Because of these scenarios, I think having a boolean return value is useful. what do you think?
3. Can't come up with better name. Feel free to suggest :)
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (PIG-1292) Interface Refinements
Posted by "Ashutosh Chauhan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ashutosh Chauhan updated PIG-1292:
----------------------------------
Attachment: pig-interfaces.patch
A preview patch with suggested changes.
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (PIG-1292) Interface Refinements
Posted by "Xuefu Zhang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12844604#action_12844604 ]
Xuefu Zhang commented on PIG-1292:
----------------------------------
If the response to #2 is the assumption, then don't create a new interface with a single method in it. Put this method in any existing interfaces, say, as an abstract method in LoadFunc itself. If a loader is absolutely incapable, then return false always. Otherwise, if the loader's answer varies case-by-case, then return true/false selectively in the implementation.
In general, I think an API should be simple and clear. I can image that other developers may have the same difficulty understanding what's this interface about.
FYI, Zebra TableLoader now extends one parent class, and implements FIVE interfaces. With this trend, it may grow even more. To me, this seems excessive.
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (PIG-1292) Interface Refinements
Posted by "Ashutosh Chauhan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12844632#action_12844632 ]
Ashutosh Chauhan commented on PIG-1292:
---------------------------------------
One reason for not putting it in LoadFunc is to keep loadfunc simple and not have such highly specific methods in there. We want to move such specialized capabilities away from LoadFunc into their own interfaces. This is also the reason PIG-966 decided to split LoadFunc into separate interfaces like LoadPushDown, LoadCaster, LoadMetaData etc. and not put all of them in LoadFunc. This frees loadfunc implementers from not thinking about them, if they don't want to. And if one wants to have such specific capability in his loader, he has to think about it anyway whether its in loadfunc or in its own interface.
That said, I agree having boolean return value for the method seems to be confusing, so I agree method return value should be void.
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (PIG-1292) Interface Refinements
Posted by "Hadoop QA (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12845336#action_12845336 ]
Hadoop QA commented on PIG-1292:
--------------------------------
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12438638/pig-1292.patch
against trunk revision 923043.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 6 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
-1 release audit. The applied patch generated 531 release audit warnings (more than the trunk's current 530 warnings).
-1 core tests. The patch failed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Pig-Patch-h7.grid.sp2.yahoo.net/237/testReport/
Release audit warnings: http://hudson.zones.apache.org/hudson/job/Pig-Patch-h7.grid.sp2.yahoo.net/237/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Pig-Patch-h7.grid.sp2.yahoo.net/237/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: http://hudson.zones.apache.org/hudson/job/Pig-Patch-h7.grid.sp2.yahoo.net/237/console
This message is automatically generated.
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-1292.patch, pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (PIG-1292) Interface Refinements
Posted by "Ashutosh Chauhan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ashutosh Chauhan reassigned PIG-1292:
-------------------------------------
Assignee: Ashutosh Chauhan
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (PIG-1292) Interface Refinements
Posted by "Xuefu Zhang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12844724#action_12844724 ]
Xuefu Zhang commented on PIG-1292:
----------------------------------
Looking at the OrderedLoadFunc interface, public WritableComparable<?> getSplitComparable(InputSplit split, int splitIdx), I am not sure why split index suddenly comes into the picture. Though it was in earlier discussion between Pig and Zebra, we agree that this is very implementation specific, which shouldn't dictate API design. Thus, I don't think that split index should be in the signature even if it helps Zebra implementation. If an implementation needs the split index, it can always store the index in the split it generates. That's what exactly Zebra plan to do.
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-1292.patch, pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (PIG-1292) Interface Refinements
Posted by "Ashutosh Chauhan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ashutosh Chauhan updated PIG-1292:
----------------------------------
Attachment: pig-1292.patch
Didn't get about LoadMetaData, ResourceSchema. LoadMetaData is one of those interfaces which loaders can choose to implement. ResourceSchema is independent class of its own.
New patch incorporating suggested changes in the above comments. This patch also adds checks in the MRCompiler to enforce loader to implement new CollectableLoader interface if there is a map-side grouping ( PIG-984 ) in the script.
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-1292.patch, pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (PIG-1292) Interface Refinements
Posted by "Ashutosh Chauhan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ashutosh Chauhan updated PIG-1292:
----------------------------------
Status: Patch Available (was: Open)
Hudson is fickle recently. Hopefully, this patch gets lucky and is tested correctly.
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-1292.patch, pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (PIG-1292) Interface Refinements
Posted by "Pradeep Kamath (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12845717#action_12845717 ]
Pradeep Kamath commented on PIG-1292:
-------------------------------------
As Xuefu mentioned, we can get rid of the splitIdx argument in public WritableComparable<?> getSplitComparable(InputSplit split, int splitIdx).
Otherwise the changes look good, +1 for commit with the above change.
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-1292.patch, pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (PIG-1292) Interface Refinements
Posted by "Ashutosh Chauhan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ashutosh Chauhan updated PIG-1292:
----------------------------------
Resolution: Fixed
Status: Resolved (was: Patch Available)
Patch checked-in with changes suggested in previous comment. Core test failure reported by hudson was transient. It passed on my machine.
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.7.0
>
> Attachments: pig-1292.patch, pig-interfaces.patch
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (PIG-1292) Interface Refinements
Posted by "Ashutosh Chauhan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/PIG-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12844247#action_12844247 ]
Ashutosh Chauhan commented on PIG-1292:
---------------------------------------
Currently LoadFunc is an abstract class. OrderedLoadFunc is another abstract class which extends LoadFunc and adds the method which tells Pig in what order to read the splits. Similarly, there is IndexableLoadFunc which also extends LoadFunc and adds the functionality that loader can arbitrarily seek near to specified keys. Its not hard to imagine that there may exist a loader which can do both. Currently there can't be such a loader since both of these are abstract classes. Proposal is to change them to interfaces.
Further, a loader may also provide a guarantee that all instances of a key appear together in one split. A similar loader is required for map-side groups PIG-984 . Currently, its assumed that underlying loader is providing data in a way its expected. We should formalize this assumption by introducing new interface and checking if loader is implementing it.
> Interface Refinements
> ---------------------
>
> Key: PIG-1292
> URL: https://issues.apache.org/jira/browse/PIG-1292
> Project: Pig
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Ashutosh Chauhan
> Fix For: 0.7.0
>
>
> A loader can't implement both OrderedLoadFunc and IndexableLoadFunc, as both are abstract classes instead of being interfaces.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.