You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by "Ning Zhang (JIRA)" <ji...@apache.org> on 2010/05/18 19:50:42 UTC
[jira] Created: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Moving inputFileChanged() from ExecMapper to where it is needed
---------------------------------------------------------------
Key: HIVE-1348
URL: https://issues.apache.org/jira/browse/HIVE-1348
Project: Hadoop Hive
Issue Type: Improvement
Reporter: Ning Zhang
Assignee: He Yongqiang
inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "Ning Zhang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12869224#action_12869224 ]
Ning Zhang commented on HIVE-1348:
----------------------------------
Looks good in general. A few questions:
1) what is the reason to change ExecMapperContext to ExecMapperLocalContext? Is the former only used in MapJoin?
2) ExecMapper.memoryMXBean is made a public static member and it is only used in ExecMapperLocalContext. Is it really needed to report the memory usage there? If not necessary we can keep it as private.
3) Do you need to pass ExecMapper instance to ExecMapperLocalContext? It seems it is only needed for setting the abort status. Instead of doing that, we probably should throw an exception in processMapLocalWork and the upper stream operators and eventually ExecMapper will catch that and set the abort due to the exceptions.
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "He Yongqiang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
He Yongqiang updated HIVE-1348:
-------------------------------
Attachment: hive-1348.3.patch
Incorporates Ning's comments. Thanks Ning.
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch, hive-1348.3.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "Namit Jain (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Namit Jain resolved HIVE-1348.
------------------------------
Hadoop Flags: [Reviewed]
Resolution: Fixed
Committed. Thanks Yongqiang//Ning
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch, hive-1348.3.patch, HIVE-1348.4.patch, HIVE-1348.5.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "Ning Zhang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ning Zhang updated HIVE-1348:
-----------------------------
Attachment: HIVE-1348.5.patch
Good points Yongqiang. I'm uploading a new patch addressing these 2 issues.
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch, hive-1348.3.patch, HIVE-1348.4.patch, HIVE-1348.5.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "Carl Steinbach (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Carl Steinbach updated HIVE-1348:
---------------------------------
Fix Version/s: 0.6.0
Component/s: Query Processor
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Components: Query Processor
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Fix For: 0.6.0
>
> Attachments: hive-1348.1.patch, hive-1348.2.patch, hive-1348.3.patch, HIVE-1348.4.patch, HIVE-1348.5.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "He Yongqiang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12869246#action_12869246 ]
He Yongqiang commented on HIVE-1348:
------------------------------------
1.
I changed ExecMapperContext to ExecMapperLocalContext because right now it is only used for map joins. But i can revert it because 'ExecMapperContext ' is more general and can be used later for other cases.
2.
Yes. We need to use ExecMapper.memoryMXBean to report memory usage in ExecMapperLocalContext. This is because it can be used to track memory usage for mapjoin's local work. And once ExecMapper.memoryMXBean is public, it can also be used in other places.
3.
will do it.
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "Ning Zhang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ning Zhang updated HIVE-1348:
-----------------------------
Attachment: HIVE-1348.4.patch
Since Yongqiang is tied up with other tasks, I'm uploading a new patch HIVE-1348.4.patch to simplify the ExecMapperContext and the logic to check if the input file has been changed.
It differs from the previous version in the following way:
1) the lastInputFile in ExecMapperContext will be only modified by resetRow() which should be called only once for each new row by the root of the operator tree -- ExecMapper.map(). It should not be changed by other operators throughout the operator tree.
2) removed the variable inputFileChanged in ExecMapperContext and simplified the function inputFileChanged() so that it can be called by any operator in the operator tree, and can be called multiple times.
3) the currentInputFile will be updated only by inputFileChanged(). If the function is not called, the variable doesn't need to be updated.
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch, hive-1348.3.patch, HIVE-1348.4.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "Namit Jain (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12875347#action_12875347 ]
Namit Jain commented on HIVE-1348:
----------------------------------
The fix for https://issues.apache.org/jira/browse/HIVE-1388 can be better insulated at CombineFileRecordReader.
So, there will not be any conflicts with this - will commit this one right now.
+1
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch, hive-1348.3.patch, HIVE-1348.4.patch, HIVE-1348.5.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "He Yongqiang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12870124#action_12870124 ]
He Yongqiang commented on HIVE-1348:
------------------------------------
Yeah, I agree. However, the event of input file changed can only be emitted one time. If we store it in one operator, other operators may miss this event. So that's why we maintain a kind of global variable for all operators.
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch, hive-1348.3.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "He Yongqiang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
He Yongqiang updated HIVE-1348:
-------------------------------
Attachment: hive-1348.1.patch
Patch for review.
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "He Yongqiang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12869299#action_12869299 ]
He Yongqiang commented on HIVE-1348:
------------------------------------
1.
We do not want to check the conf 2 times to see if the input file has changed or not. So that's why the variable inputFileChanged is used for. maybe we should give a better name to 'inputFileChanged()' ( checkInputFileChanged() ?)
2.
i will change the variable name.
3.
No. they will not change the mapjoin behavior. Those code will only be executed one time for normal mapjoin.
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "Namit Jain (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12874881#action_12874881 ]
Namit Jain commented on HIVE-1348:
----------------------------------
Please dont merge it yet - we may have a urgent usecase for which we might need whether input file changed or not for all combineHiveInputFormat queries.
I am trying to create a test, will file a jira once I have a concrete testcase
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch, hive-1348.3.patch, HIVE-1348.4.patch, HIVE-1348.5.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "Ning Zhang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12869289#action_12869289 ]
Ning Zhang commented on HIVE-1348:
----------------------------------
some details:
1) SMBMapJoinOperator and ExecMapperContext: why not just call the function inputFileChanged() rather than maintaining another variable with the same name inputFileChanged? There is not benefit in doing that and only make it more complicated.
2) ExecMapper:72, execLocalContext --> execContext, variable name is not consistent.
3) MapJoin:163-167: won't these lines change the semantics of what the mapjoin operator did previously?
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "Ning Zhang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12874948#action_12874948 ]
Ning Zhang commented on HIVE-1348:
----------------------------------
@namit, please note that with this patch, you can call ExecMapperContext.inputFileChanged() at any operator to check if the current row is from a different file than the last row.
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch, hive-1348.3.patch, HIVE-1348.4.patch, HIVE-1348.5.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "He Yongqiang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12874646#action_12874646 ]
He Yongqiang commented on HIVE-1348:
------------------------------------
Ning, thanks for updating!
Overall looks good to me. Some minor comments:
1. in MapjoinOperator
if (tag == posBigTable && this.getExecContext().getLastInputFile() == null) {
we may need to remove "this.getExecContext().getLastInputFile() == null" because of bucket mapjoin
2. i did no see where we set the var "inputFileChecked" in ExecMapperContext to true.
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch, hive-1348.3.patch, HIVE-1348.4.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "He Yongqiang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
He Yongqiang updated HIVE-1348:
-------------------------------
Attachment: hive-1348.2.patch
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HIVE-1348) Moving inputFileChanged() from
ExecMapper to where it is needed
Posted by "Ning Zhang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HIVE-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12870088#action_12870088 ]
Ning Zhang commented on HIVE-1348:
----------------------------------
Yongqiang, I still think we should just call the function inputFileChanged() or checkInputFileChanged(). The reasons are:
1) input file change could change record over record. So the only case you want to cache it is inside the processOp() itself, not cross rows. So caching the variable in ExecMapperContext doesn't make sense since it is shared across all operators and all iterations. Also making the variable inputFileChanged public and let other class change it doesn't make sense and could easily lead incorrect result.
2) The only use of the variable ExecMapperContext.inputFileChanged is in SMBMapJoinOperator.procssOp() and close(). It doesn't need to be cached in ExecMapperContext (or even in a local class variable) at all IMO. Can you explain why?
> Moving inputFileChanged() from ExecMapper to where it is needed
> ---------------------------------------------------------------
>
> Key: HIVE-1348
> URL: https://issues.apache.org/jira/browse/HIVE-1348
> Project: Hadoop Hive
> Issue Type: Improvement
> Reporter: Ning Zhang
> Assignee: He Yongqiang
> Attachments: hive-1348.1.patch, hive-1348.2.patch, hive-1348.3.patch
>
>
> inputFileChanged() is only needed for Bucketed sort merge map join. It should not be put in ExecMapper.map() where all code paths will hit this function. This function is quite expensive since JobConf look up is a hash table look up.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.