You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Veena Basavaraj <vb...@cloudera.com> on 2014/12/06 04:35:29 UTC
Review Request 28782: Make DateTime Column type support datetime with
and without timezone
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28782/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOO-1845
https://issues.apache.org/jira/browse/SQOO-1845
Repository: sqoop-sqoop2
Description
-------
see jira
Diffs
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java 2f5fadc
common/src/main/java/org/apache/sqoop/schema/type/DateTime.java 791542a
common/src/main/java/org/apache/sqoop/schema/type/Time.java 9765eab
common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 3d31600
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java 9d50dc3
connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java daa51eb
connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java e66c897
core/src/main/java/org/apache/sqoop/driver/JobManager.java f4f5561
Diff: https://reviews.apache.org/r/28782/diff/
Testing
-------
yes
Thanks,
Veena Basavaraj
Re: Review Request 28782: Make DateTime Column type support datetime
with and without timezone
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28782/
-----------------------------------------------------------
(Updated Dec. 8, 2014, 11:33 a.m.)
Review request for Sqoop.
Bugs: SQOO-1845
https://issues.apache.org/jira/browse/SQOO-1845
Repository: sqoop-sqoop2
Description
-------
see jira
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java 2f5fadc
common/src/main/java/org/apache/sqoop/schema/type/DateTime.java 791542a
common/src/main/java/org/apache/sqoop/schema/type/Time.java 9765eab
common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 3d31600
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java 9d50dc3
connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java daa51eb
connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java e66c897
Diff: https://reviews.apache.org/r/28782/diff/
Testing
-------
yes
Thanks,
Veena Basavaraj
Re: Review Request 28782: Make DateTime Column type support datetime
with and without timezone
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28782/
-----------------------------------------------------------
(Updated Dec. 8, 2014, 11:31 a.m.)
Review request for Sqoop.
Bugs: SQOO-1845
https://issues.apache.org/jira/browse/SQOO-1845
Repository: sqoop-sqoop2
Description
-------
see jira
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java 2f5fadc
common/src/main/java/org/apache/sqoop/schema/type/DateTime.java 791542a
common/src/main/java/org/apache/sqoop/schema/type/Time.java 9765eab
common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 3d31600
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java 9d50dc3
connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java daa51eb
connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java e66c897
Diff: https://reviews.apache.org/r/28782/diff/
Testing
-------
yes
Thanks,
Veena Basavaraj
Re: Review Request 28782: Make DateTime Column type support datetime
with and without timezone
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Dec. 7, 2014, 11:29 a.m., Abraham Elmahrek wrote:
> > LGTM.
> >
> > IMO the most important part of the story is being able to add to this over time. For instance, maybe we can support multiple formats of dates, times, and timestamps in text eventually. Also, we should be able to add a transformation layer that can remove timezone information if needed if we so choose to make all timestamps that don't include timezone information simply have UTC offset.
Sure, need more elaborate definition and design, i'd like to see a proposal for this for future.
> On Dec. 7, 2014, 11:29 a.m., Abraham Elmahrek wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java, line 70
> > <https://reviews.apache.org/r/28782/diff/1/?file=784391#file784391line70>
> >
> > TIMESTAMP should have timezone information?
not sure the line matches, did not understand the comment
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28782/#review64186
-----------------------------------------------------------
On Dec. 5, 2014, 7:35 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28782/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 7:35 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOO-1845
> https://issues.apache.org/jira/browse/SQOO-1845
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java 2f5fadc
> common/src/main/java/org/apache/sqoop/schema/type/DateTime.java 791542a
> common/src/main/java/org/apache/sqoop/schema/type/Time.java 9765eab
> common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 3d31600
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java 9d50dc3
> connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java daa51eb
> connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java e66c897
> core/src/main/java/org/apache/sqoop/driver/JobManager.java f4f5561
>
> Diff: https://reviews.apache.org/r/28782/diff/
>
>
> Testing
> -------
>
> yes
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 28782: Make DateTime Column type support datetime
with and without timezone
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28782/#review64186
-----------------------------------------------------------
Ship it!
LGTM.
IMO the most important part of the story is being able to add to this over time. For instance, maybe we can support multiple formats of dates, times, and timestamps in text eventually. Also, we should be able to add a transformation layer that can remove timezone information if needed if we so choose to make all timestamps that don't include timezone information simply have UTC offset.
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java
<https://reviews.apache.org/r/28782/#comment106655>
TIMESTAMP should have timezone information?
core/src/main/java/org/apache/sqoop/driver/JobManager.java
<https://reviews.apache.org/r/28782/#comment106654>
Probably not necessary. The Joda jar will be included when calling jobRequest.addJarForClass(LocalDate.class)
- Abraham Elmahrek
On Dec. 6, 2014, 3:35 a.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28782/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2014, 3:35 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOO-1845
> https://issues.apache.org/jira/browse/SQOO-1845
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java 2f5fadc
> common/src/main/java/org/apache/sqoop/schema/type/DateTime.java 791542a
> common/src/main/java/org/apache/sqoop/schema/type/Time.java 9765eab
> common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 3d31600
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java 9d50dc3
> connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java daa51eb
> connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java e66c897
> core/src/main/java/org/apache/sqoop/driver/JobManager.java f4f5561
>
> Diff: https://reviews.apache.org/r/28782/diff/
>
>
> Testing
> -------
>
> yes
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 28782: Make DateTime Column type support datetime
with and without timezone
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28782/#review64257
-----------------------------------------------------------
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java
<https://reviews.apache.org/r/28782/#comment106894>
timestamp does not have it.
Jarcec confirmed it as well.
- Veena Basavaraj
On Dec. 5, 2014, 7:35 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28782/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 7:35 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOO-1845
> https://issues.apache.org/jira/browse/SQOO-1845
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java 2f5fadc
> common/src/main/java/org/apache/sqoop/schema/type/DateTime.java 791542a
> common/src/main/java/org/apache/sqoop/schema/type/Time.java 9765eab
> common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 3d31600
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java 9d50dc3
> connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java daa51eb
> connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java e66c897
> core/src/main/java/org/apache/sqoop/driver/JobManager.java f4f5561
>
> Diff: https://reviews.apache.org/r/28782/diff/
>
>
> Testing
> -------
>
> yes
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 28782: Make DateTime Column type support datetime
with and without timezone
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Dec. 7, 2014, 8:37 p.m., Qian Xu wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, line 402
> > <https://reviews.apache.org/r/28782/diff/1/?file=784393#file784393line402>
> >
> > The minutes second and millis parts are always zero. I'd suggest construct a random datetime value (i.e. `now`) as fixture, so that they will all get tested.
> >
> > DateTime fixture = DateTime.now()
> > dateFormat.setTextData(fixture.toString("YYYY-mm-dd HH:MM:SS.zz");
> > assertEquals(fixture.toString(), dateFormat.getObjectData()[0].toString());
> >
> > Nits: I'd suggest remove three note comments.
whats wrong with the notes :)
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28782/#review64192
-----------------------------------------------------------
On Dec. 5, 2014, 7:35 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28782/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 7:35 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOO-1845
> https://issues.apache.org/jira/browse/SQOO-1845
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java 2f5fadc
> common/src/main/java/org/apache/sqoop/schema/type/DateTime.java 791542a
> common/src/main/java/org/apache/sqoop/schema/type/Time.java 9765eab
> common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 3d31600
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java 9d50dc3
> connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java daa51eb
> connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java e66c897
> core/src/main/java/org/apache/sqoop/driver/JobManager.java f4f5561
>
> Diff: https://reviews.apache.org/r/28782/diff/
>
>
> Testing
> -------
>
> yes
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 28782: Make DateTime Column type support datetime
with and without timezone
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Dec. 7, 2014, 8:37 p.m., Qian Xu wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, line 402
> > <https://reviews.apache.org/r/28782/diff/1/?file=784393#file784393line402>
> >
> > The minutes second and millis parts are always zero. I'd suggest construct a random datetime value (i.e. `now`) as fixture, so that they will all get tested.
> >
> > DateTime fixture = DateTime.now()
> > dateFormat.setTextData(fixture.toString("YYYY-mm-dd HH:MM:SS.zz");
> > assertEquals(fixture.toString(), dateFormat.getObjectData()[0].toString());
> >
> > Nits: I'd suggest remove three note comments.
>
> Veena Basavaraj wrote:
> whats wrong with the notes :)
>
> Qian Xu wrote:
> What is the propose of the test case then? Does it check, whether a DateTime field is able to be stored as a datetime object (in our case a joda datetime)? I thought we expect `dataFormat.getObjectData()[0]` to be a `org.jodatime.DateTime` object and the value equals to the expected datetime value.
>
> Instead of
>
> // NOTE: string representation will have the T added, it is an
> // implementation quirk of using JODA
> assertEquals("2014-10-01T12:00:00.000-04:00", dataFormat.getObjectData()[0].toString());
>
> My first thought of the test code would be
>
> DateTimeZone zone = DateTimeZone.forID("America/New_York");
> org.joda.time.DateTime expectedDateTime = new org.joda.time.DateTime(2014, 10, 01, 12, 0, 0, 0, zone);
> assertTrue(dataFormat.getTextData() instanceof new org.joda.time.DateTime);
> assertEquals(expectedDateTime, dataFormat.getObjectData()[0]);
it covers both the object and string, see the line before tha note that asserts the object.
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28782/#review64192
-----------------------------------------------------------
On Dec. 8, 2014, 11:33 a.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28782/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2014, 11:33 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOO-1845
> https://issues.apache.org/jira/browse/SQOO-1845
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java 2f5fadc
> common/src/main/java/org/apache/sqoop/schema/type/DateTime.java 791542a
> common/src/main/java/org/apache/sqoop/schema/type/Time.java 9765eab
> common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 3d31600
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java 9d50dc3
> connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java daa51eb
> connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java e66c897
>
> Diff: https://reviews.apache.org/r/28782/diff/
>
>
> Testing
> -------
>
> yes
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 28782: Make DateTime Column type support datetime
with and without timezone
Posted by Qian Xu <sx...@googlemail.com>.
> On Dec. 8, 2014, 12:37 p.m., Qian Xu wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, line 402
> > <https://reviews.apache.org/r/28782/diff/1/?file=784393#file784393line402>
> >
> > The minutes second and millis parts are always zero. I'd suggest construct a random datetime value (i.e. `now`) as fixture, so that they will all get tested.
> >
> > DateTime fixture = DateTime.now()
> > dateFormat.setTextData(fixture.toString("YYYY-mm-dd HH:MM:SS.zz");
> > assertEquals(fixture.toString(), dateFormat.getObjectData()[0].toString());
> >
> > Nits: I'd suggest remove three note comments.
>
> Veena Basavaraj wrote:
> whats wrong with the notes :)
What is the propose of the test case then? Does it check, whether a DateTime field is able to be stored as a datetime object (in our case a joda datetime)? I thought we expect `dataFormat.getObjectData()[0]` to be a `org.jodatime.DateTime` object and the value equals to the expected datetime value.
Instead of
// NOTE: string representation will have the T added, it is an
// implementation quirk of using JODA
assertEquals("2014-10-01T12:00:00.000-04:00", dataFormat.getObjectData()[0].toString());
My first thought of the test code would be
DateTimeZone zone = DateTimeZone.forID("America/New_York");
org.joda.time.DateTime expectedDateTime = new org.joda.time.DateTime(2014, 10, 01, 12, 0, 0, 0, zone);
assertTrue(dataFormat.getTextData() instanceof new org.joda.time.DateTime);
assertEquals(expectedDateTime, dataFormat.getObjectData()[0]);
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28782/#review64192
-----------------------------------------------------------
On Dec. 9, 2014, 3:33 a.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28782/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 3:33 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOO-1845
> https://issues.apache.org/jira/browse/SQOO-1845
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java 2f5fadc
> common/src/main/java/org/apache/sqoop/schema/type/DateTime.java 791542a
> common/src/main/java/org/apache/sqoop/schema/type/Time.java 9765eab
> common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 3d31600
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java 9d50dc3
> connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java daa51eb
> connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java e66c897
>
> Diff: https://reviews.apache.org/r/28782/diff/
>
>
> Testing
> -------
>
> yes
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 28782: Make DateTime Column type support datetime
with and without timezone
Posted by Qian Xu <sx...@googlemail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28782/#review64192
-----------------------------------------------------------
connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java
<https://reviews.apache.org/r/28782/#comment106813>
The minutes second and millis parts are always zero. I'd suggest construct a random datetime value (i.e. `now`) as fixture, so that they will all get tested.
DateTime fixture = DateTime.now()
dateFormat.setTextData(fixture.toString("YYYY-mm-dd HH:MM:SS.zz");
assertEquals(fixture.toString(), dateFormat.getObjectData()[0].toString());
Nits: I'd suggest remove three note comments.
- Qian Xu
On Dec. 6, 2014, 11:35 a.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28782/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2014, 11:35 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOO-1845
> https://issues.apache.org/jira/browse/SQOO-1845
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java 2f5fadc
> common/src/main/java/org/apache/sqoop/schema/type/DateTime.java 791542a
> common/src/main/java/org/apache/sqoop/schema/type/Time.java 9765eab
> common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 3d31600
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java 9d50dc3
> connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java daa51eb
> connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java e66c897
> core/src/main/java/org/apache/sqoop/driver/JobManager.java f4f5561
>
> Diff: https://reviews.apache.org/r/28782/diff/
>
>
> Testing
> -------
>
> yes
>
>
> Thanks,
>
> Veena Basavaraj
>
>