hadoop-pig-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Gates (JIRA)" <j...@apache.org>
Subject [jira] Commented: (PIG-161) Rework physical plan
Date Tue, 01 Apr 2008 19:14:30 GMT

    [ https://issues.apache.org/jira/browse/PIG-161?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12584258#action_12584258

Alan Gates commented on PIG-161:

Alan's reponses to Shravan's respones to Alan's comments:


1.1 Filter's compOperand should always return type Boolean. The type checker will
assure this. If in the future we decide to be C like and allow expressions
like: "filter b by $0" implying that $0 is non-null or non-zero or something
than it will be the task of the type checker to put a cast in there to make it
[shrav] You are right. But for the getNext calls to cascade we need to know the result type
of the operands of compOp as they can be anything. Hence, instead of using the result type
of the comparison operator, I use the operands' result type.
Alan>> I don't understand what you mean by cascade.  Can you give me an example of how
this would work and why you need to use the result type of the operands of compOp instead
of it's return type?

1.2 Line 88, why is it an error for the predecessor to pass a NULL?
[shrav] I am not sure I understand this. So are you saying that the NULL returned should be
passed up the pipeline? I thought that we had agreed on not passing NULLs through the pipeline.
Alan>> It need not be passed up the pipeline.  But you are logging a warning when you
receive it.  I don't think we need that.  If the production of the null was an error, then
the point at which the null was generated should have issued the appropriate warning.  

1.3 When processing the results from the compOperand, it calls continue if
compOperand returns any status besides OK. Why? Shouldn't it pass on errors?
compOperand could also legitimately return NULL, in which case continue is the correct
[shrav] I have implemented it in a greedy fashion. It tries to avoid passing errors on as
it is happening only inside its scope and tries in hope that the next tuple processed will
not throw an error. If this is wrong, I can change it to pass errors on. I have followed the
same philosphy everywhere.
Alan>> But the issue is that you'll swallow errors.  If compOp return ERR, you should
be stopping processing, not ignoring the error.


2.2 Default implementations of getNext() should return ERR, not null.
[shrav] It does return ERR I thought. It returns a new instance of Result which has the returnStatus
set to ERR.
Alan>> My error, I missed that res is set to an empty Result in the constructor.


4.1 Why is getNext() with no arguments public? Do you anticipate outsiders
calling it? It seems like it should be private.
[shrav] It was private before. I neede to use it in the unit tests instead of calling getNext
with each data type. I guess I was lazy. Should I change this?
Alan>> It's okay if we need it for the test.  Ideally the tests should be in the same
package, and then this could be a package level call.  Right now we don't seem to be doing
our junit tests that way.


5.1 visitGreaterThan should visit its constituent expressions (ie
visit.getLhs, visit.getRhs).
[shrav] I guess this was what I was suggesting for the logical operator as well. I maintain
a separate plan for expressions. The ExprPlan consists of all the expression operators in
the greater than expression and ExprPlanVisitor is used only for expressions. So the visit
will automatically visit lhs and rhs and probably another input like for a ternary one as
Alan>> Ok.


6.1 visitFilter should visit its comparison operator.
[shrav] I thought this should be left to some concrete implementation of the PhyPlanVisitor.
Alan>> Concrete implementations may choose to override it, but the default should be
to visit it.

> Rework physical plan
> --------------------
>                 Key: PIG-161
>                 URL: https://issues.apache.org/jira/browse/PIG-161
>             Project: Pig
>          Issue Type: Sub-task
>            Reporter: Alan Gates
>            Assignee: Alan Gates
>         Attachments: Phy_AbsClass.patch, pogenerate.patch
> This bug tracks work to rework all of the physical operators as described in http://wiki.apache.org/pig/PigTypesFunctionalSpec

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message