You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by cheng xu <ch...@intel.com> on 2014/12/11 06:57:46 UTC
Review Request 28933: HIVE-8131:Support timestamp in Avro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28933/
-----------------------------------------------------------
Review request for hive.
Repository: hive-git
Description
-------
The patch includes:
1.add timestamp support for AvroSerde
2.add related test cases
Diffs
-----
data/files/avro_timestamp.txt PRE-CREATION
ql/src/test/queries/clientpositive/avro_timestamp.q PRE-CREATION
ql/src/test/results/clientpositive/avro_timestamp.q.out PRE-CREATION
serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroDeserializer.java 07c5ecf
serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerDe.java 7639a2b
serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerializer.java c8eac89
serde/src/java/org/apache/hadoop/hive/serde2/avro/SchemaToTypeInfo.java c84b1a0
serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java 8cb2dc3
serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java cd5a0fa
Diff: https://reviews.apache.org/r/28933/diff/
Testing
-------
Test passed for added cases
Thanks,
cheng xu
Re: Review Request 28933: HIVE-8131:Support timestamp in Avro
Posted by Mohit Sabharwal <mo...@cloudera.com>.
> On Dec. 14, 2014, 7:50 a.m., Mohit Sabharwal wrote:
> > ql/src/test/results/clientpositive/avro_timestamp.q.out, line 101
> > <https://reviews.apache.org/r/28933/diff/1/?file=789140#file789140line101>
> >
> > Looks like we are truncating the timestamp from millis to nanos because Avro spec expects millis.
> >
> > Any idea why the precision for the partition timestamp column is printed in nanos whenever timestamp is in where clause ?
> >
> > Also maybe add another query in the test with millis in the where clause ?
Typo... I meant "truncating the timestamp from nanos to millis "
- Mohit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28933/#review65039
-----------------------------------------------------------
On Dec. 11, 2014, 5:57 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28933/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 5:57 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1.add timestamp support for AvroSerde
> 2.add related test cases
>
>
> Diffs
> -----
>
> data/files/avro_timestamp.txt PRE-CREATION
> ql/src/test/queries/clientpositive/avro_timestamp.q PRE-CREATION
> ql/src/test/results/clientpositive/avro_timestamp.q.out PRE-CREATION
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroDeserializer.java 07c5ecf
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerDe.java 7639a2b
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerializer.java c8eac89
> serde/src/java/org/apache/hadoop/hive/serde2/avro/SchemaToTypeInfo.java c84b1a0
> serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java 8cb2dc3
> serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java cd5a0fa
>
> Diff: https://reviews.apache.org/r/28933/diff/
>
>
> Testing
> -------
>
> Test passed for added cases
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28933: HIVE-8131:Support timestamp in Avro
Posted by cheng xu <ch...@intel.com>.
> On Dec. 14, 2014, 7:50 a.m., Mohit Sabharwal wrote:
> > ql/src/test/results/clientpositive/avro_timestamp.q.out, line 101
> > <https://reviews.apache.org/r/28933/diff/1/?file=789140#file789140line101>
> >
> > Looks like we are truncating the timestamp from millis to nanos because Avro spec expects millis.
> >
> > Any idea why the precision for the partition timestamp column is printed in nanos whenever timestamp is in where clause ?
> >
> > Also maybe add another query in the test with millis in the where clause ?
>
> Mohit Sabharwal wrote:
> Typo... I meant "truncating the timestamp from nanos to millis "
This following entry may be helpful. Timestamp is typically in the precision of millis. Do you think there is any special requirement to improve the precision for partition? For the test case, maybe we can use timestamp in the precision of millis instead to avoid confusing.
See http://svn.apache.org/repos/asf/openjpa/trunk/openjpa-lib/src/main/java/org/apache/openjpa/lib/util/TimestampHelper.java
- cheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28933/#review65039
-----------------------------------------------------------
On Dec. 11, 2014, 5:57 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28933/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 5:57 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1.add timestamp support for AvroSerde
> 2.add related test cases
>
>
> Diffs
> -----
>
> data/files/avro_timestamp.txt PRE-CREATION
> ql/src/test/queries/clientpositive/avro_timestamp.q PRE-CREATION
> ql/src/test/results/clientpositive/avro_timestamp.q.out PRE-CREATION
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroDeserializer.java 07c5ecf
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerDe.java 7639a2b
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerializer.java c8eac89
> serde/src/java/org/apache/hadoop/hive/serde2/avro/SchemaToTypeInfo.java c84b1a0
> serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java 8cb2dc3
> serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java cd5a0fa
>
> Diff: https://reviews.apache.org/r/28933/diff/
>
>
> Testing
> -------
>
> Test passed for added cases
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28933: HIVE-8131:Support timestamp in Avro
Posted by Mohit Sabharwal <mo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28933/#review65039
-----------------------------------------------------------
ql/src/test/results/clientpositive/avro_timestamp.q.out
<https://reviews.apache.org/r/28933/#comment107977>
Looks like we are truncating the timestamp from millis to nanos because Avro spec expects millis.
Any idea why the precision for the partition timestamp column is printed in nanos whenever timestamp is in where clause ?
Also maybe add another query in the test with millis in the where clause ?
serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerDe.java
<https://reviews.apache.org/r/28933/#comment107976>
should be timestamp-milllis per the spec...
serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java
<https://reviews.apache.org/r/28933/#comment107975>
Shouldn't the logical type be "timestamp-millis" according to Avro spec ?
See: https://issues.apache.org/jira/secure/attachment/12663245/AVRO-739-update-spec.diff
serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java
<https://reviews.apache.org/r/28933/#comment107974>
"Test for timestamp" instead of "Test for date"
- Mohit Sabharwal
On Dec. 11, 2014, 5:57 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28933/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 5:57 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1.add timestamp support for AvroSerde
> 2.add related test cases
>
>
> Diffs
> -----
>
> data/files/avro_timestamp.txt PRE-CREATION
> ql/src/test/queries/clientpositive/avro_timestamp.q PRE-CREATION
> ql/src/test/results/clientpositive/avro_timestamp.q.out PRE-CREATION
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroDeserializer.java 07c5ecf
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerDe.java 7639a2b
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerializer.java c8eac89
> serde/src/java/org/apache/hadoop/hive/serde2/avro/SchemaToTypeInfo.java c84b1a0
> serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java 8cb2dc3
> serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java cd5a0fa
>
> Diff: https://reviews.apache.org/r/28933/diff/
>
>
> Testing
> -------
>
> Test passed for added cases
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28933: HIVE-8131:Support timestamp in Avro
Posted by Mohit Sabharwal <mo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28933/#review65179
-----------------------------------------------------------
Thanks for the changes! LGTM. Could you please update the patch on the JIRA as well?
- Mohit Sabharwal
On Dec. 16, 2014, 3:40 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28933/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 3:40 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1.add timestamp support for AvroSerde
> 2.add related test cases
>
>
> Diffs
> -----
>
> data/files/avro_timestamp.txt PRE-CREATION
> ql/src/test/queries/clientpositive/avro_timestamp.q PRE-CREATION
> ql/src/test/results/clientpositive/avro_timestamp.q.out PRE-CREATION
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroDeserializer.java 07c5ecf
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerDe.java 7639a2b
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerializer.java c8eac89
> serde/src/java/org/apache/hadoop/hive/serde2/avro/SchemaToTypeInfo.java c84b1a0
> serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java 8cb2dc3
> serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java cd5a0fa
>
> Diff: https://reviews.apache.org/r/28933/diff/
>
>
> Testing
> -------
>
> Test passed for added cases
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28933: HIVE-8131:Support timestamp in Avro
Posted by Ryan Blue <bl...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28933/#review65334
-----------------------------------------------------------
Ship it!
Ship It!
- Ryan Blue
On Dec. 15, 2014, 7:40 p.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28933/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2014, 7:40 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1.add timestamp support for AvroSerde
> 2.add related test cases
>
>
> Diffs
> -----
>
> data/files/avro_timestamp.txt PRE-CREATION
> ql/src/test/queries/clientpositive/avro_timestamp.q PRE-CREATION
> ql/src/test/results/clientpositive/avro_timestamp.q.out PRE-CREATION
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroDeserializer.java 07c5ecf
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerDe.java 7639a2b
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerializer.java c8eac89
> serde/src/java/org/apache/hadoop/hive/serde2/avro/SchemaToTypeInfo.java c84b1a0
> serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java 8cb2dc3
> serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java cd5a0fa
>
> Diff: https://reviews.apache.org/r/28933/diff/
>
>
> Testing
> -------
>
> Test passed for added cases
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28933: HIVE-8131:Support timestamp in Avro
Posted by cheng xu <ch...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28933/
-----------------------------------------------------------
(Updated Dec. 16, 2014, 3:40 a.m.)
Review request for hive.
Changes
-------
Summary for the updates:
1. change the type from timestamp to timestamp-millis
2. change the data's precision from nanos to millis
3. clean code
Repository: hive-git
Description
-------
The patch includes:
1.add timestamp support for AvroSerde
2.add related test cases
Diffs (updated)
-----
data/files/avro_timestamp.txt PRE-CREATION
ql/src/test/queries/clientpositive/avro_timestamp.q PRE-CREATION
ql/src/test/results/clientpositive/avro_timestamp.q.out PRE-CREATION
serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroDeserializer.java 07c5ecf
serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerDe.java 7639a2b
serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerializer.java c8eac89
serde/src/java/org/apache/hadoop/hive/serde2/avro/SchemaToTypeInfo.java c84b1a0
serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java 8cb2dc3
serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java cd5a0fa
Diff: https://reviews.apache.org/r/28933/diff/
Testing
-------
Test passed for added cases
Thanks,
cheng xu
Re: Review Request 28933: HIVE-8131:Support timestamp in Avro
Posted by cheng xu <ch...@intel.com>.
> On Dec. 12, 2014, 4:30 a.m., Dong Chen wrote:
> > data/files/avro_timestamp.txt, line 4
> > <https://reviews.apache.org/r/28933/diff/1/?file=789138#file789138line4>
> >
> > how about add some other formats?
We can use timestamp with the precision of millis instead.
> On Dec. 12, 2014, 4:30 a.m., Dong Chen wrote:
> > serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java, line 268
> > <https://reviews.apache.org/r/28933/diff/1/?file=789146#file789146line268>
> >
> > We check TypeInfo -> Schema here. Can we also check Schema -> TypeInfo ?
This may be out of the scope of TestTypeInfoToSchema.java.
- cheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28933/#review64857
-----------------------------------------------------------
On Dec. 11, 2014, 5:57 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28933/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 5:57 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1.add timestamp support for AvroSerde
> 2.add related test cases
>
>
> Diffs
> -----
>
> data/files/avro_timestamp.txt PRE-CREATION
> ql/src/test/queries/clientpositive/avro_timestamp.q PRE-CREATION
> ql/src/test/results/clientpositive/avro_timestamp.q.out PRE-CREATION
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroDeserializer.java 07c5ecf
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerDe.java 7639a2b
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerializer.java c8eac89
> serde/src/java/org/apache/hadoop/hive/serde2/avro/SchemaToTypeInfo.java c84b1a0
> serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java 8cb2dc3
> serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java cd5a0fa
>
> Diff: https://reviews.apache.org/r/28933/diff/
>
>
> Testing
> -------
>
> Test passed for added cases
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28933: HIVE-8131:Support timestamp in Avro
Posted by Dong Chen <do...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28933/#review64857
-----------------------------------------------------------
Looks good! I left some minor comments below.
data/files/avro_timestamp.txt
<https://reviews.apache.org/r/28933/#comment107655>
how about add some other formats?
serde/src/java/org/apache/hadoop/hive/serde2/avro/SchemaToTypeInfo.java
<https://reviews.apache.org/r/28933/#comment107656>
It is consistent in the code to use "Schema.Type.Long". Then the import is not necessary.
serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java
<https://reviews.apache.org/r/28933/#comment107657>
We check TypeInfo -> Schema here. Can we also check Schema -> TypeInfo ?
- Dong Chen
On Dec. 11, 2014, 5:57 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28933/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 5:57 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1.add timestamp support for AvroSerde
> 2.add related test cases
>
>
> Diffs
> -----
>
> data/files/avro_timestamp.txt PRE-CREATION
> ql/src/test/queries/clientpositive/avro_timestamp.q PRE-CREATION
> ql/src/test/results/clientpositive/avro_timestamp.q.out PRE-CREATION
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroDeserializer.java 07c5ecf
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerDe.java 7639a2b
> serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerializer.java c8eac89
> serde/src/java/org/apache/hadoop/hive/serde2/avro/SchemaToTypeInfo.java c84b1a0
> serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java 8cb2dc3
> serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java cd5a0fa
>
> Diff: https://reviews.apache.org/r/28933/diff/
>
>
> Testing
> -------
>
> Test passed for added cases
>
>
> Thanks,
>
> cheng xu
>
>