You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Jitendra Pandey <ji...@hortonworks.com> on 2014/02/12 20:00:46 UTC
Review Request 18025: Implement vectorized support for COALESCE conditional
expression
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18025/
-----------------------------------------------------------
Review request for hive and Eric Hanson.
Bugs: HIVE-5759
https://issues.apache.org/jira/browse/HIVE-5759
Repository: hive-git
Description
-------
Implement vectorized support for COALESCE conditional expression
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/BytesColumnVector.java f1eef14
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/ColumnVector.java 0a8811f
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DecimalColumnVector.java d0d8597
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DoubleColumnVector.java cb23129
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/LongColumnVector.java aa05b19
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java 7141d63
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorCoalesce.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java 21fe8ca
ql/src/test/queries/clientpositive/vector_coalesce.q PRE-CREATION
ql/src/test/results/clientpositive/vector_coalesce.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/18025/diff/
Testing
-------
Thanks,
Jitendra Pandey
Re: Review Request 18025: Implement vectorized support for COALESCE
conditional expression
Posted by Jitendra Pandey <ji...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18025/
-----------------------------------------------------------
(Updated Feb. 14, 2014, 1:49 a.m.)
Review request for hive and Eric Hanson.
Bugs: HIVE-5759
https://issues.apache.org/jira/browse/HIVE-5759
Repository: hive-git
Description
-------
Implement vectorized support for COALESCE conditional expression
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/BytesColumnVector.java f1eef14
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/ColumnVector.java 0a8811f
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DecimalColumnVector.java d0d8597
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DoubleColumnVector.java cb23129
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/LongColumnVector.java aa05b19
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java 7141d63
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorCoalesce.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java 21fe8ca
ql/src/test/queries/clientpositive/vector_coalesce.q PRE-CREATION
ql/src/test/results/clientpositive/vector_coalesce.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/18025/diff/
Testing
-------
Thanks,
Jitendra Pandey
Re: Review Request 18025: Implement vectorized support for COALESCE
conditional expression
Posted by Eric Hanson <eh...@microsoft.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18025/#review34370
-----------------------------------------------------------
ql/src/test/queries/clientpositive/vector_coalesce.q
<https://reviews.apache.org/r/18025/#comment64447>
Can you do one with > 3 arguments too? Will that vectorize?
ql/src/test/queries/clientpositive/vector_coalesce.q
<https://reviews.apache.org/r/18025/#comment64450>
Please also test for smallint and timestamp.
ql/src/test/queries/clientpositive/vector_coalesce.q
<https://reviews.apache.org/r/18025/#comment64451>
Please also test for expressions as arguments, not just columns.
ql/src/test/queries/clientpositive/vector_coalesce.q
<https://reviews.apache.org/r/18025/#comment64448>
It is not unusual to use COALESCE like this:
COALESCE(col1, ..., colK, 0)
So if arguments 1..K are NULL, the default value is the constant at the end, 0 in this case. Could you please make that work in this patch, or open a separate JIRA to do it later?
- Eric Hanson
On Feb. 12, 2014, 7 p.m., Jitendra Pandey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18025/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2014, 7 p.m.)
>
>
> Review request for hive and Eric Hanson.
>
>
> Bugs: HIVE-5759
> https://issues.apache.org/jira/browse/HIVE-5759
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Implement vectorized support for COALESCE conditional expression
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/BytesColumnVector.java f1eef14
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/ColumnVector.java 0a8811f
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DecimalColumnVector.java d0d8597
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DoubleColumnVector.java cb23129
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/LongColumnVector.java aa05b19
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java 7141d63
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorCoalesce.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java 21fe8ca
> ql/src/test/queries/clientpositive/vector_coalesce.q PRE-CREATION
> ql/src/test/results/clientpositive/vector_coalesce.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/18025/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jitendra Pandey
>
>
Re: Review Request 18025: Implement vectorized support for COALESCE
conditional expression
Posted by Jitendra Pandey <ji...@hortonworks.com>.
> On Feb. 13, 2014, 1:30 a.m., Eric Hanson wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java, line 440
> > <https://reviews.apache.org/r/18025/diff/1/?file=483651#file483651line440>
> >
> > This is the same code block as the previous case. Can you share the case and change the condition to an OR?
> >
> > Up to you...
This piece of code will be revisited in HIVE-6416, where I need to add casting for many mathematical functions. I will address it there.
> On Feb. 13, 2014, 1:30 a.m., Eric Hanson wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorCoalesce.java, line 126
> > <https://reviews.apache.org/r/18025/diff/1/?file=483652#file483652line126>
> >
> > This can have > 1 argument. Please add comment to explain.
> >
> >
This class should not return a descriptor, because it can take variable number of arguments with different data types.
- Jitendra
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18025/#review34336
-----------------------------------------------------------
On Feb. 14, 2014, 1:49 a.m., Jitendra Pandey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18025/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2014, 1:49 a.m.)
>
>
> Review request for hive and Eric Hanson.
>
>
> Bugs: HIVE-5759
> https://issues.apache.org/jira/browse/HIVE-5759
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Implement vectorized support for COALESCE conditional expression
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/BytesColumnVector.java f1eef14
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/ColumnVector.java 0a8811f
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DecimalColumnVector.java d0d8597
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DoubleColumnVector.java cb23129
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/LongColumnVector.java aa05b19
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java 7141d63
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorCoalesce.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java 21fe8ca
> ql/src/test/queries/clientpositive/vector_coalesce.q PRE-CREATION
> ql/src/test/results/clientpositive/vector_coalesce.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/18025/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jitendra Pandey
>
>
Re: Review Request 18025: Implement vectorized support for COALESCE
conditional expression
Posted by Eric Hanson <eh...@microsoft.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18025/#review34336
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/BytesColumnVector.java
<https://reviews.apache.org/r/18025/#comment64388>
I think setRef is only safe for base vectors (that get data from table columns), not intermediate working results. There was bug there since those can get re-used during processing of a single vectorized row batch.
So, use setVal here unless you know the source vector is a base vector loaded from a table column.
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DecimalColumnVector.java
<https://reviews.apache.org/r/18025/#comment64389>
use .update() instead of = assignment or you could have a bug.
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java
<https://reviews.apache.org/r/18025/#comment64392>
please add comment to explain what method does
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java
<https://reviews.apache.org/r/18025/#comment64395>
This is the same code block as the previous case. Can you share the case and change the condition to an OR?
Up to you...
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorCoalesce.java
<https://reviews.apache.org/r/18025/#comment64396>
I'm not sure this is always EOF. Consider deleting ", this is EOF"
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorCoalesce.java
<https://reviews.apache.org/r/18025/#comment64397>
This can have > 1 argument. Please add comment to explain.
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorCoalesce.java
<https://reviews.apache.org/r/18025/#comment64398>
What happens if one of the inputs is a scalar, not a column?
ql/src/test/queries/clientpositive/vector_coalesce.q
<https://reviews.apache.org/r/18025/#comment64399>
ERIC TODO: start reviewing here
- Eric Hanson
On Feb. 12, 2014, 7 p.m., Jitendra Pandey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18025/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2014, 7 p.m.)
>
>
> Review request for hive and Eric Hanson.
>
>
> Bugs: HIVE-5759
> https://issues.apache.org/jira/browse/HIVE-5759
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Implement vectorized support for COALESCE conditional expression
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/BytesColumnVector.java f1eef14
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/ColumnVector.java 0a8811f
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DecimalColumnVector.java d0d8597
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DoubleColumnVector.java cb23129
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/LongColumnVector.java aa05b19
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java 7141d63
> ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorCoalesce.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java 21fe8ca
> ql/src/test/queries/clientpositive/vector_coalesce.q PRE-CREATION
> ql/src/test/results/clientpositive/vector_coalesce.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/18025/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jitendra Pandey
>
>