You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Teddy Choi <pu...@gmail.com> on 2018/05/31 02:47:14 UTC
Re: Review Request 65174: [HIVE-17896] TopNKey: Create a standalone
vectorizable TopNKey operator
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65174/
-----------------------------------------------------------
(Updated 5 31, 2018, 2:47 오전)
Review request for hive.
Changes
-------
I rebased the patch on the latest master branch.
Bugs: HIVE-17896
https://issues.apache.org/jira/browse/HIVE-17896
Repository: hive-git
Description
-------
For TPC-DS Query27, the TopN operation is delayed by the group-by - the group-by operator buffers up all the rows before discarding the 99% of the rows in the TopN Hash within the ReduceSink Operator.
The RS TopN operator is very restrictive as it only supports doing the filtering on the shuffle keys, but it is better to do this before breaking the vectors into rows and losing the isRepeating properties.
Adding a TopN Key operator in the physical operator tree allows the following to happen.
GBY->RS(Top=1)
can become
TNK(1)->GBY->RS(Top=1)
So that, the TopNKey can remove rows before they are buffered into the GBY and consume memory.
Here's the equivalent implementation in Presto
https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/TopNOperator.java#L35
Adding this as a sub-feature of GroupBy prevents further optimizations if the GBY is on keys "a,b,c" and the TopNKey is on just "a".
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3295d1dbc5
itests/src/test/resources/testconfiguration.properties 6a70a4a6bd
ql/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/ql/plan/api/OperatorType.java a002348013
ql/src/java/org/apache/hadoop/hive/ql/exec/KeyWrapperFactory.java 3c7f0b78c2
ql/src/java/org/apache/hadoop/hive/ql/exec/OperatorFactory.java 7bb6590d5e
ql/src/java/org/apache/hadoop/hive/ql/exec/TopNKeyOperator.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorTopNKeyOperator.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyPushdown.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java 394f826508
ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java dfd790853b
ql/src/java/org/apache/hadoop/hive/ql/plan/TopNKeyDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/plan/VectorTopNKeyDesc.java PRE-CREATION
ql/src/test/queries/clientpositive/topnkey.q PRE-CREATION
ql/src/test/queries/clientpositive/vector_topnkey.q PRE-CREATION
ql/src/test/results/clientpositive/llap/topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/llap/vector_topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/tez/topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/tez/vector_topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/vector_topnkey.q.out PRE-CREATION
serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorUtils.java a442cb1228
Diff: https://reviews.apache.org/r/65174/diff/2/
Changes: https://reviews.apache.org/r/65174/diff/1-2/
Testing
-------
Thanks,
Teddy Choi
Re: Review Request 65174: [HIVE-17896] TopNKey: Create a standalone
vectorizable TopNKey operator
Posted by Jesús Camacho Rodríguez <jc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65174/#review206027
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java
Lines 37 (patched)
<https://reviews.apache.org/r/65174/#comment288987>
I was talking to Gopal. We were thinking that for first implementation we could limit the processor to introduce new TopNKeyOp only below RS-Gby(HASH mode). So basically 1) match RS with GBy below, 2) check whether RS contains TopN, 3) check whether GBy is in hash mode, and 4) check whether RS keys are same as GBy keys. Then, if condition is met, introduce topN below GBy.
When we work on pushdown in follow-up, we can generalize this process, extend it, etc.
- Jesús Camacho Rodríguez
On July 11, 2018, 12:30 p.m., Teddy Choi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65174/
> -----------------------------------------------------------
>
> (Updated July 11, 2018, 12:30 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-17896
> https://issues.apache.org/jira/browse/HIVE-17896
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> For TPC-DS Query27, the TopN operation is delayed by the group-by - the group-by operator buffers up all the rows before discarding the 99% of the rows in the TopN Hash within the ReduceSink Operator.
> The RS TopN operator is very restrictive as it only supports doing the filtering on the shuffle keys, but it is better to do this before breaking the vectors into rows and losing the isRepeating properties.
> Adding a TopN Key operator in the physical operator tree allows the following to happen.
> GBY->RS(Top=1)
> can become
> TNK(1)->GBY->RS(Top=1)
> So that, the TopNKey can remove rows before they are buffered into the GBY and consume memory.
> Here's the equivalent implementation in Presto
> https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/TopNOperator.java#L35
> Adding this as a sub-feature of GroupBy prevents further optimizations if the GBY is on keys "a,b,c" and the TopNKey is on just "a".
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6ea68c3500
> itests/src/test/resources/testconfiguration.properties 9e012ce2f8
> ql/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/ql/plan/api/OperatorType.java a002348013
> ql/src/java/org/apache/hadoop/hive/ql/exec/KeyWrapperFactory.java 71ee25d9e0
> ql/src/java/org/apache/hadoop/hive/ql/exec/OperatorFactory.java 7bb6590d5e
> ql/src/java/org/apache/hadoop/hive/ql/exec/TopNKeyOperator.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorTopNKeyOperator.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java 7afbf04797
> ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java dfd790853b
> ql/src/java/org/apache/hadoop/hive/ql/plan/TopNKeyDesc.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/plan/VectorTopNKeyDesc.java PRE-CREATION
> ql/src/test/queries/clientpositive/topnkey.q PRE-CREATION
> ql/src/test/queries/clientpositive/vector_topnkey.q PRE-CREATION
> ql/src/test/results/clientpositive/llap/topnkey.q.out PRE-CREATION
> ql/src/test/results/clientpositive/llap/vector_topnkey.q.out PRE-CREATION
> ql/src/test/results/clientpositive/tez/topnkey.q.out PRE-CREATION
> ql/src/test/results/clientpositive/tez/vector_topnkey.q.out PRE-CREATION
> ql/src/test/results/clientpositive/topnkey.q.out PRE-CREATION
> ql/src/test/results/clientpositive/vector_topnkey.q.out PRE-CREATION
> serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorUtils.java 9393fb853f
>
>
> Diff: https://reviews.apache.org/r/65174/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Teddy Choi
>
>
Re: Review Request 65174: [HIVE-17896] TopNKey: Create a standalone
vectorizable TopNKey operator
Posted by Teddy Choi <pu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65174/
-----------------------------------------------------------
(Updated July 17, 2018, 9:16 a.m.)
Review request for hive.
Changes
-------
Applied Jesus' feedbacks. Thanks!
Bugs: HIVE-17896
https://issues.apache.org/jira/browse/HIVE-17896
Repository: hive-git
Description
-------
For TPC-DS Query27, the TopN operation is delayed by the group-by - the group-by operator buffers up all the rows before discarding the 99% of the rows in the TopN Hash within the ReduceSink Operator.
The RS TopN operator is very restrictive as it only supports doing the filtering on the shuffle keys, but it is better to do this before breaking the vectors into rows and losing the isRepeating properties.
Adding a TopN Key operator in the physical operator tree allows the following to happen.
GBY->RS(Top=1)
can become
TNK(1)->GBY->RS(Top=1)
So that, the TopNKey can remove rows before they are buffered into the GBY and consume memory.
Here's the equivalent implementation in Presto
https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/TopNOperator.java#L35
Adding this as a sub-feature of GroupBy prevents further optimizations if the GBY is on keys "a,b,c" and the TopNKey is on just "a".
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 858c63011b
itests/src/test/resources/testconfiguration.properties d08528f319
ql/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/ql/plan/api/OperatorType.java a002348013
ql/src/java/org/apache/hadoop/hive/ql/exec/KeyWrapperFactory.java 71ee25d9e0
ql/src/java/org/apache/hadoop/hive/ql/exec/OperatorFactory.java 7bb6590d5e
ql/src/java/org/apache/hadoop/hive/ql/exec/TopNKeyOperator.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorTopNKeyOperator.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java 7ec80e6132
ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 1b433c7498
ql/src/java/org/apache/hadoop/hive/ql/plan/TopNKeyDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/plan/VectorTopNKeyDesc.java PRE-CREATION
ql/src/test/queries/clientpositive/topnkey.q PRE-CREATION
ql/src/test/queries/clientpositive/vector_topnkey.q PRE-CREATION
ql/src/test/results/clientpositive/llap/topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/llap/vector_topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/tez/topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/tez/vector_topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/vector_topnkey.q.out PRE-CREATION
serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorUtils.java 9393fb853f
Diff: https://reviews.apache.org/r/65174/diff/4/
Changes: https://reviews.apache.org/r/65174/diff/3-4/
Testing
-------
Thanks,
Teddy Choi
Re: Review Request 65174: [HIVE-17896] TopNKey: Create a standalone
vectorizable TopNKey operator
Posted by Jesús Camacho Rodríguez <jc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65174/#review205972
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
Lines 2248 (patched)
<https://reviews.apache.org/r/65174/#comment288920>
Let's enable it by default: though it will be a pain because of the qfile changes, it will help us discover any existing issues.
When we pair it with 1) a pushdown rule, and 2) a redundant topN removal rule, this should always result in better plans.
ql/src/java/org/apache/hadoop/hive/ql/exec/TopNKeyOperator.java
Lines 37 (patched)
<https://reviews.apache.org/r/65174/#comment288914>
Can we add a short description for the operator?
ql/src/java/org/apache/hadoop/hive/ql/exec/TopNKeyOperator.java
Lines 42 (patched)
<https://reviews.apache.org/r/65174/#comment288915>
Can we add comments for this properties?
ql/src/java/org/apache/hadoop/hive/ql/exec/TopNKeyOperator.java
Lines 181 (patched)
<https://reviews.apache.org/r/65174/#comment288921>
I think this should be _false_ as skew join optimization is only supported in few specific cases, and hence no need to override it? If it should be true, add a comment explaining it; otherwise, please remove it.
ql/src/java/org/apache/hadoop/hive/ql/exec/TopNKeyOperator.java
Lines 185 (patched)
<https://reviews.apache.org/r/65174/#comment288922>
Same as above, this is only _true_ currently for FilterOp and SelectOp. If this should be _true_ here too, please provide comment. Otherwise, we do not need to override.
ql/src/java/org/apache/hadoop/hive/ql/exec/TopNKeyOperator.java
Lines 190 (patched)
<https://reviews.apache.org/r/65174/#comment288923>
Same as above, better remove it and enable it afterwards if needed unless we are absolutely sure this is correct.
ql/src/java/org/apache/hadoop/hive/ql/exec/TopNKeyOperator.java
Lines 195 (patched)
<https://reviews.apache.org/r/65174/#comment288924>
Same as above, better remove it and enable it afterwards if needed unless we are absolutely sure this is correct.
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorTopNKeyOperator.java
Lines 266 (patched)
<https://reviews.apache.org/r/65174/#comment288930>
Same as for TopNKeyOperator, we should be cautious.
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorTopNKeyOperator.java
Lines 271 (patched)
<https://reviews.apache.org/r/65174/#comment288931>
Same as for TopNKeyOperator, we should be cautious.
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorTopNKeyOperator.java
Lines 276 (patched)
<https://reviews.apache.org/r/65174/#comment288932>
Same as for TopNKeyOperator, we should be cautious.
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorTopNKeyOperator.java
Lines 281 (patched)
<https://reviews.apache.org/r/65174/#comment288933>
Same as for TopNKeyOperator, we should be cautious.
ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java
Lines 36 (patched)
<https://reviews.apache.org/r/65174/#comment288934>
Add general comment to the class.
ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java
Lines 59 (patched)
<https://reviews.apache.org/r/65174/#comment288941>
Parent operator can be extracted from _reduceSinkOperator.getParentOperators().get(0)_.
ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java
Lines 60 (patched)
<https://reviews.apache.org/r/65174/#comment288942>
Can we add condition _if parentOp instanceof TopK, bail out_? Safer in case we end up visiting same op more than once.
ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java
Lines 60 (patched)
<https://reviews.apache.org/r/65174/#comment288943>
Can we add condition _if parentOp instanceof TopN, bail out_? Safer in case we visit an operator more than once.
ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java
Lines 61 (patched)
<https://reviews.apache.org/r/65174/#comment288939>
This should be equal to _nd_ and _reduceSinkOperator_, no need to create a third copy.
ql/src/java/org/apache/hadoop/hive/ql/plan/TopNKeyDesc.java
Lines 101 (patched)
<https://reviews.apache.org/r/65174/#comment288944>
We should use _ExprNodeDescUtils.isSame_ for the expression columns (as in _ReduceSinkDesc_).
- Jesús Camacho Rodríguez
On July 11, 2018, 12:30 p.m., Teddy Choi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65174/
> -----------------------------------------------------------
>
> (Updated July 11, 2018, 12:30 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-17896
> https://issues.apache.org/jira/browse/HIVE-17896
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> For TPC-DS Query27, the TopN operation is delayed by the group-by - the group-by operator buffers up all the rows before discarding the 99% of the rows in the TopN Hash within the ReduceSink Operator.
> The RS TopN operator is very restrictive as it only supports doing the filtering on the shuffle keys, but it is better to do this before breaking the vectors into rows and losing the isRepeating properties.
> Adding a TopN Key operator in the physical operator tree allows the following to happen.
> GBY->RS(Top=1)
> can become
> TNK(1)->GBY->RS(Top=1)
> So that, the TopNKey can remove rows before they are buffered into the GBY and consume memory.
> Here's the equivalent implementation in Presto
> https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/TopNOperator.java#L35
> Adding this as a sub-feature of GroupBy prevents further optimizations if the GBY is on keys "a,b,c" and the TopNKey is on just "a".
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6ea68c3500
> itests/src/test/resources/testconfiguration.properties 9e012ce2f8
> ql/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/ql/plan/api/OperatorType.java a002348013
> ql/src/java/org/apache/hadoop/hive/ql/exec/KeyWrapperFactory.java 71ee25d9e0
> ql/src/java/org/apache/hadoop/hive/ql/exec/OperatorFactory.java 7bb6590d5e
> ql/src/java/org/apache/hadoop/hive/ql/exec/TopNKeyOperator.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorTopNKeyOperator.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java 7afbf04797
> ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java dfd790853b
> ql/src/java/org/apache/hadoop/hive/ql/plan/TopNKeyDesc.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/plan/VectorTopNKeyDesc.java PRE-CREATION
> ql/src/test/queries/clientpositive/topnkey.q PRE-CREATION
> ql/src/test/queries/clientpositive/vector_topnkey.q PRE-CREATION
> ql/src/test/results/clientpositive/llap/topnkey.q.out PRE-CREATION
> ql/src/test/results/clientpositive/llap/vector_topnkey.q.out PRE-CREATION
> ql/src/test/results/clientpositive/tez/topnkey.q.out PRE-CREATION
> ql/src/test/results/clientpositive/tez/vector_topnkey.q.out PRE-CREATION
> ql/src/test/results/clientpositive/topnkey.q.out PRE-CREATION
> ql/src/test/results/clientpositive/vector_topnkey.q.out PRE-CREATION
> serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorUtils.java 9393fb853f
>
>
> Diff: https://reviews.apache.org/r/65174/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Teddy Choi
>
>
Re: Review Request 65174: [HIVE-17896] TopNKey: Create a standalone
vectorizable TopNKey operator
Posted by Teddy Choi <pu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65174/
-----------------------------------------------------------
(Updated 7 11, 2018, 12:30 오후)
Review request for hive.
Changes
-------
Removed plan optimizer part and updated the output files.
Bugs: HIVE-17896
https://issues.apache.org/jira/browse/HIVE-17896
Repository: hive-git
Description
-------
For TPC-DS Query27, the TopN operation is delayed by the group-by - the group-by operator buffers up all the rows before discarding the 99% of the rows in the TopN Hash within the ReduceSink Operator.
The RS TopN operator is very restrictive as it only supports doing the filtering on the shuffle keys, but it is better to do this before breaking the vectors into rows and losing the isRepeating properties.
Adding a TopN Key operator in the physical operator tree allows the following to happen.
GBY->RS(Top=1)
can become
TNK(1)->GBY->RS(Top=1)
So that, the TopNKey can remove rows before they are buffered into the GBY and consume memory.
Here's the equivalent implementation in Presto
https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/TopNOperator.java#L35
Adding this as a sub-feature of GroupBy prevents further optimizations if the GBY is on keys "a,b,c" and the TopNKey is on just "a".
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6ea68c3500
itests/src/test/resources/testconfiguration.properties 9e012ce2f8
ql/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/ql/plan/api/OperatorType.java a002348013
ql/src/java/org/apache/hadoop/hive/ql/exec/KeyWrapperFactory.java 71ee25d9e0
ql/src/java/org/apache/hadoop/hive/ql/exec/OperatorFactory.java 7bb6590d5e
ql/src/java/org/apache/hadoop/hive/ql/exec/TopNKeyOperator.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorTopNKeyOperator.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java 7afbf04797
ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java dfd790853b
ql/src/java/org/apache/hadoop/hive/ql/plan/TopNKeyDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/plan/VectorTopNKeyDesc.java PRE-CREATION
ql/src/test/queries/clientpositive/topnkey.q PRE-CREATION
ql/src/test/queries/clientpositive/vector_topnkey.q PRE-CREATION
ql/src/test/results/clientpositive/llap/topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/llap/vector_topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/tez/topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/tez/vector_topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/topnkey.q.out PRE-CREATION
ql/src/test/results/clientpositive/vector_topnkey.q.out PRE-CREATION
serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorUtils.java 9393fb853f
Diff: https://reviews.apache.org/r/65174/diff/3/
Changes: https://reviews.apache.org/r/65174/diff/2-3/
Testing
-------
Thanks,
Teddy Choi