You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Zoltan Haindrich <ki...@rxd.hu> on 2018/03/14 16:31:15 UTC
Review Request 66069: HIVE-18926 Imporve operator-tree matching
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66069/
-----------------------------------------------------------
Review request for hive and Ashutosh Chauhan.
Bugs: HIVE-18926
https://issues.apache.org/jira/browse/HIVE-18926
Repository: hive-git
Description
-------
* match operators based on opsigs
* always calc operator sigs
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/OpSignature.java 90b2fd3dad8ee9ec14213d286d05ef21f53d45a0
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/OpTreeSignature.java c6d1a6aaca27bc487654b41612073fb2e20c2023
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/Signature.java c228a8e4f52f31deb5781a5b875ca5c521722ec7
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/SignatureUtils.java 2269322b69ab0d003d7ebc83a6784267477e5551
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java 22b052cd07928fb0011787424b5af7b75dcc6bf4
ql/src/java/org/apache/hadoop/hive/ql/plan/CommonMergeJoinDesc.java 5a81add7066c4d886bf30e2c50c71194763174db
ql/src/java/org/apache/hadoop/hive/ql/plan/JoinCondDesc.java ea22131c56aede0b523d861808214520f031ea2f
ql/src/java/org/apache/hadoop/hive/ql/plan/LateralViewJoinDesc.java 85a4683491d6f22bdea1c40b042fedcc24cfbf31
ql/src/java/org/apache/hadoop/hive/ql/plan/MapJoinDesc.java 91ea159d524771b4492b3ff3bb402deb37e106d4
ql/src/java/org/apache/hadoop/hive/ql/plan/ReduceSinkDesc.java f2955af25ee09fa0521da0cb601181ceb9d18417
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java 36d7e589a844359e1e91e5150541298f8cac837f
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapperProcess.java 424dd7956b9e50bc7729289a889452a07b59351a
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/RuntimeStatsSource.java 21a06781533b684269f88daa7f1c9522383b1971
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/SimpleRuntimeStatsSource.java 6f340b8450a95569156b3b7eddf28fdd53fc9065
ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java 93031712dc6fb60a6d618c8754e50def489a12af
ql/src/test/org/apache/hadoop/hive/ql/optimizer/signature/TestOperatorSignature.java 8c899e7fef1a1ebe06fdd9269f3c922f8baab84d
ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestOperatorCmp.java cfb0ca38b2289c29ff93bc8cfd6868de17ed4fad
ql/src/test/results/clientpositive/llap/retry_failure_stat_changes.q.out 9b58ce0e5d99a704c4a6305487e937b2ea727624
Diff: https://reviews.apache.org/r/66069/diff/1/
Testing
-------
Thanks,
Zoltan Haindrich
Re: Review Request 66069: HIVE-18926 Imporve operator-tree matching
Posted by Zoltan Haindrich <ki...@rxd.hu>.
> On March 20, 2018, 12:08 a.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java
> > Line 2501 (original), 2507 (patched)
> > <https://reviews.apache.org/r/66069/diff/2/?file=1982591#file1982591line2507>
> >
> > I think lookup(op) is better than lookup(treeSig) since that way we can delay computation of signature. e.g, if I configure to run with EmptyStatsSource (to turn off this feature), this computation will be avoided.
the operator to signature calculation is done using a cache using the PlanMapper which is bound to the current plan;
If I change the interface method to use Operator - I loose access to that signature cache - and the usage of the cache will probably look a bit out of place during the lookup process - I think this way it's more natural that the signature cache will go down along with the mapper.
I think it would be probably safer this way in the long run - since in case of a statsSource which has a longer lifetime than a single query , it may by mistake keep some references to the previous query's objects thru some cache...
I've added an conditional to the beginning of this method to disable these things altogether in case reexecution is disabled.
> On March 20, 2018, 12:08 a.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java
> > Line 43 (original), 48 (patched)
> > <https://reviews.apache.org/r/66069/diff/2/?file=1982597#file1982597line48>
> >
> > Add comments about this class.
I've added some apidocs; and I've renamed it to EquivGroup
- Zoltan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66069/#review199495
-----------------------------------------------------------
On March 19, 2018, 9:59 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66069/
> -----------------------------------------------------------
>
> (Updated March 19, 2018, 9:59 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Bugs: HIVE-18926
> https://issues.apache.org/jira/browse/HIVE-18926
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> * match operators based on opsigs
> * always calc operator sigs
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java a235f3fbf4
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/OpSignature.java 90b2fd3dad
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/OpTreeSignature.java c6d1a6aaca
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/Signature.java c228a8e4f5
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/SignatureUtils.java 2269322b69
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java 22b052cd07
> ql/src/java/org/apache/hadoop/hive/ql/plan/CommonMergeJoinDesc.java 5a81add706
> ql/src/java/org/apache/hadoop/hive/ql/plan/JoinCondDesc.java ea22131c56
> ql/src/java/org/apache/hadoop/hive/ql/plan/LateralViewJoinDesc.java 85a4683491
> ql/src/java/org/apache/hadoop/hive/ql/plan/MapJoinDesc.java 91ea159d52
> ql/src/java/org/apache/hadoop/hive/ql/plan/ReduceSinkDesc.java f2955af25e
> ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java 36d7e589a8
> ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapperProcess.java 424dd7956b
> ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/RuntimeStatsSource.java 21a0678153
> ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/SimpleRuntimeStatsSource.java 6f340b8450
> ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java 93031712dc
> ql/src/test/org/apache/hadoop/hive/ql/optimizer/signature/TestOperatorSignature.java 8c899e7fef
> ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestOperatorCmp.java cfb0ca38b2
> ql/src/test/results/clientpositive/llap/retry_failure_stat_changes.q.out 9b58ce0e5d
>
>
> Diff: https://reviews.apache.org/r/66069/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zoltan Haindrich
>
>
Re: Review Request 66069: HIVE-18926 Imporve operator-tree matching
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66069/#review199495
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java
Lines 381 (patched)
<https://reviews.apache.org/r/66069/#comment279799>
Can you please add a comment here that what we are doing here and why?
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java
Line 2501 (original), 2507 (patched)
<https://reviews.apache.org/r/66069/#comment279803>
I think lookup(op) is better than lookup(treeSig) since that way we can delay computation of signature. e.g, if I configure to run with EmptyStatsSource (to turn off this feature), this computation will be avoided.
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java
Line 43 (original), 48 (patched)
<https://reviews.apache.org/r/66069/#comment279801>
Add comments about this class.
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java
Line 63 (original), 71 (patched)
<https://reviews.apache.org/r/66069/#comment279802>
Comments.
- Ashutosh Chauhan
On March 19, 2018, 9:59 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66069/
> -----------------------------------------------------------
>
> (Updated March 19, 2018, 9:59 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Bugs: HIVE-18926
> https://issues.apache.org/jira/browse/HIVE-18926
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> * match operators based on opsigs
> * always calc operator sigs
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java a235f3fbf4
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/OpSignature.java 90b2fd3dad
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/OpTreeSignature.java c6d1a6aaca
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/Signature.java c228a8e4f5
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/SignatureUtils.java 2269322b69
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java 22b052cd07
> ql/src/java/org/apache/hadoop/hive/ql/plan/CommonMergeJoinDesc.java 5a81add706
> ql/src/java/org/apache/hadoop/hive/ql/plan/JoinCondDesc.java ea22131c56
> ql/src/java/org/apache/hadoop/hive/ql/plan/LateralViewJoinDesc.java 85a4683491
> ql/src/java/org/apache/hadoop/hive/ql/plan/MapJoinDesc.java 91ea159d52
> ql/src/java/org/apache/hadoop/hive/ql/plan/ReduceSinkDesc.java f2955af25e
> ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java 36d7e589a8
> ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapperProcess.java 424dd7956b
> ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/RuntimeStatsSource.java 21a0678153
> ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/SimpleRuntimeStatsSource.java 6f340b8450
> ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java 93031712dc
> ql/src/test/org/apache/hadoop/hive/ql/optimizer/signature/TestOperatorSignature.java 8c899e7fef
> ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestOperatorCmp.java cfb0ca38b2
> ql/src/test/results/clientpositive/llap/retry_failure_stat_changes.q.out 9b58ce0e5d
>
>
> Diff: https://reviews.apache.org/r/66069/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zoltan Haindrich
>
>
Re: Review Request 66069: HIVE-18926 Imporve operator-tree matching
Posted by Zoltan Haindrich <ki...@rxd.hu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66069/
-----------------------------------------------------------
(Updated March 21, 2018, 3:49 p.m.)
Review request for hive and Ashutosh Chauhan.
Changes
-------
05
Bugs: HIVE-18926
https://issues.apache.org/jira/browse/HIVE-18926
Repository: hive-git
Description (updated)
-------
* match operators based on opsigs
* always calc operator sigs
* fix explain reoptimization thru HS2
Diffs (updated)
-----
itests/src/test/resources/testconfiguration.properties f513fe5ff7
ql/src/java/org/apache/hadoop/hive/ql/Context.java 58fa5f2287
ql/src/java/org/apache/hadoop/hive/ql/Driver.java 75f928b69d
ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java 0a6e17ab93
ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java a235f3fbf4
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/OpSignature.java 90b2fd3dad
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/OpTreeSignature.java c6d1a6aaca
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/Signature.java c228a8e4f5
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/SignatureUtils.java 2269322b69
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java 22b052cd07
ql/src/java/org/apache/hadoop/hive/ql/plan/CommonMergeJoinDesc.java 5a81add706
ql/src/java/org/apache/hadoop/hive/ql/plan/JoinCondDesc.java ea22131c56
ql/src/java/org/apache/hadoop/hive/ql/plan/LateralViewJoinDesc.java 85a4683491
ql/src/java/org/apache/hadoop/hive/ql/plan/MapJoinDesc.java 91ea159d52
ql/src/java/org/apache/hadoop/hive/ql/plan/ReduceSinkDesc.java f2955af25e
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/EmptyStatsSource.java 57762edbdf
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/GroupTransformer.java 7b9e99e7e0
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java 36d7e589a8
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapperProcess.java 424dd7956b
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/RuntimeStatsSource.java 21a0678153
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/SimpleRuntimeStatsSource.java 6f340b8450
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/StatsSource.java a4cb6e9771
ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java 93031712dc
ql/src/java/org/apache/hadoop/hive/ql/reexec/ReOptimizePlugin.java 707858700f
ql/src/test/org/apache/hadoop/hive/ql/optimizer/signature/TestOperatorSignature.java 8c899e7fef
ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestCounterMapping.java 9fe95e4c56
ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestOperatorCmp.java cfb0ca38b2
ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestReOptimization.java 6d7bb07dc1
ql/src/test/queries/clientpositive/explain_outputs.q PRE-CREATION
ql/src/test/results/clientpositive/beeline/explain_outputs.q.out PRE-CREATION
ql/src/test/results/clientpositive/explain_outputs.q.out PRE-CREATION
ql/src/test/results/clientpositive/llap/retry_failure_stat_changes.q.out 9b58ce0e5d
service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 5865abe552
Diff: https://reviews.apache.org/r/66069/diff/3/
Changes: https://reviews.apache.org/r/66069/diff/2-3/
Testing
-------
Thanks,
Zoltan Haindrich
Re: Review Request 66069: HIVE-18926 Imporve operator-tree matching
Posted by Zoltan Haindrich <ki...@rxd.hu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66069/
-----------------------------------------------------------
(Updated March 19, 2018, 9:59 p.m.)
Review request for hive and Ashutosh Chauhan.
Changes
-------
patch#4
Bugs: HIVE-18926
https://issues.apache.org/jira/browse/HIVE-18926
Repository: hive-git
Description
-------
* match operators based on opsigs
* always calc operator sigs
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java a235f3fbf4
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/OpSignature.java 90b2fd3dad
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/OpTreeSignature.java c6d1a6aaca
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/Signature.java c228a8e4f5
ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/SignatureUtils.java 2269322b69
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java 22b052cd07
ql/src/java/org/apache/hadoop/hive/ql/plan/CommonMergeJoinDesc.java 5a81add706
ql/src/java/org/apache/hadoop/hive/ql/plan/JoinCondDesc.java ea22131c56
ql/src/java/org/apache/hadoop/hive/ql/plan/LateralViewJoinDesc.java 85a4683491
ql/src/java/org/apache/hadoop/hive/ql/plan/MapJoinDesc.java 91ea159d52
ql/src/java/org/apache/hadoop/hive/ql/plan/ReduceSinkDesc.java f2955af25e
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java 36d7e589a8
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapperProcess.java 424dd7956b
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/RuntimeStatsSource.java 21a0678153
ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/SimpleRuntimeStatsSource.java 6f340b8450
ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java 93031712dc
ql/src/test/org/apache/hadoop/hive/ql/optimizer/signature/TestOperatorSignature.java 8c899e7fef
ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestOperatorCmp.java cfb0ca38b2
ql/src/test/results/clientpositive/llap/retry_failure_stat_changes.q.out 9b58ce0e5d
Diff: https://reviews.apache.org/r/66069/diff/2/
Changes: https://reviews.apache.org/r/66069/diff/1-2/
Testing
-------
Thanks,
Zoltan Haindrich