[
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:
POFilter:
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
work.
[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
behavior.
[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.
PhysicalOperator:
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.
POProject:
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.
ExprPlanVisitor:
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 well.
Alan>> Ok.
PhyPlanVisitor:
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.
