[ 
https://issues.apache.org/jira/browse/HIVE-4502?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13709352#comment-13709352
 ] 

Phabricator commented on HIVE-4502:
-----------------------------------

ashutoshc has requested changes to the revision "HIVE-4502 [jira] NPE - 
subquery smb joins fails".

  I dont like the idea of tracking operators in the task thereby tasks getting 
involved in plan manipulation but in absence of alternatives, this may be the 
way to move forward for now.

INLINE COMMENTS
  ql/src/java/org/apache/hadoop/hive/ql/QueryPlan.java:619 I guess this is 
unintentional.
  ql/src/java/org/apache/hadoop/hive/ql/exec/Task.java:72 I am little hesistant 
about task being aware of operators. Ideally if we have good demarkation of 
different layers, task (which is sort of a driver to execute a plan) need not 
be aware of operators contained in its plan. Planner shouldn't wait so late 
that it needs to track operators in task to generate plan correctly. I don't 
have a proposal on how to do this better though. At the very least, it will 
help to add comment explaining the purpose of this list.
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMRRedSink1.java:70 I think 
its better to do following:
  If (op.getNumChild() != 1){
    throw new IllegalStateException("Expecting operator " + op + "to have one 
child. Found: " + op.getNumChild());
  }
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java:261 It 
will be good to add comments on what this method intends to do. Also, I guess 
you can make this protected, since all the callers of this method are in same 
package.
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java:281 Does 
this method need to be public?
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java:310 I 
didnt get this part. Can you add comments why old and curr tasks need to be of 
type ExecDriver. What about MapredLocalWork?
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java:318 Does 
this method need to be public? Also, please add javadocs for the method.
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java:851 throw 
IllegalStateException in case # of parents != 1
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java:949 I 
didnt get why did you remove this if check. We shall be needing tagging only 
for joins. Right? Removing this check implies we will tag always, is that 
needed?
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java:942 
Better name: hasBranchFinished() ?
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/MapJoinFactory.java:228 Why 
is this function call needed here? Can you please add comment for it?
  ql/src/test/results/clientpositive/auto_smb_mapjoin_14.q.out:165 I see that 
there is no Stage-2 at all. What caused this? Its little weird that we have 
stages 0,1 and 3 in plan and no stage-2 at all.
  ql/src/test/results/clientpositive/auto_smb_mapjoin_14.q.out:349 No stage-2 
here either. Probably the same cause?

REVISION DETAIL
  https://reviews.facebook.net/D10695

BRANCH
  HIVE-4502

ARCANIST PROJECT
  hive

To: JIRA, ashutoshc, navis
Cc: brock

                
> NPE - subquery smb joins fails
> ------------------------------
>
>                 Key: HIVE-4502
>                 URL: https://issues.apache.org/jira/browse/HIVE-4502
>             Project: Hive
>          Issue Type: Bug
>          Components: Query Processor
>    Affects Versions: 0.11.0
>            Reporter: Vikram Dixit K
>            Assignee: Navis
>         Attachments: HIVE-4502.D10695.1.patch, HIVE-4502.D10695.2.patch, 
> HIVE-4502.D10695.3.patch, smb_mapjoin_25.q, smb_mapjoin_25.q
>
>
> Found this issue while running some SMB joins. Attaching test case that 
> causes this error.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to