Re: Review Request 57551: SQOOP-2272 - Import decimal columns from mysql to hive 0.14

[Eric Lin <er...@cloudera.com>]

> On March 20, 2017, 4:07 p.m., Attila Szabo wrote:
> > Hey Eric,
> > 
> > For the first shot I do understand you motivation here, and also attached some comments/requests around the implementation.
> > 
> > Although I'm a bit concerned about the big picture as well B/C I do feel in the current form it would be a breaking change.
> > 
> > E.g. Still so far we'd created tables in hive with double columns, but after applying the change all of these imports + tables would contain DECIMAL values. Wouldn't this change confuse the end users? (for me it would be very much unexpected especially after a version upgrade!)
> > 
> > Questions on my side:
> > Am I right your change would only effect Sqoop imports when the import ends up in creating a new Hive table?
> > Wouldn't this cause problems on the end user side (e.g. in case of big incremental data ingest cases with many tables)?
> > What will happen with the jobs inserting data into an already existing Hive table? Have you tested that scenario as well?
> > Although your change seems to be much more type/Hive agnostic, shouldn't we introduce a fallback scenario for the users (e.g. turning off the Decimal->Decimal mapping with an option). Or should we just advise the explicit type mapping for the users in this case?
> > 
> > Thanks for your answers,
> > Attila
> 
> Eric Lin wrote:
>     Hi Attila,
>     
>     Thanks for your feedback. To answer your questions:
>     
>     > Am I right your change would only effect Sqoop imports when the import ends up in creating a new Hive table?
>     Yes, it only affects when creating a new table, append mode will not be affected
>     
>     > Wouldn't this cause problems on the end user side (e.g. in case of big incremental data ingest cases with many tables)?
>     I am not sure about your example here, can you give me a bit more details on your use case?
>     
>     > What will happen with the jobs inserting data into an already existing Hive table? Have you tested that scenario as well?
>     I have tested manually, the CREATE TABLE IF EXISTS will be ignored and whatever table was before will not be changed. After that it will be LOAD DATA INPATH command, so nothing will change.
>     
>     > Although your change seems to be much more type/Hive agnostic, shouldn't we introduce a fallback scenario for the users (e.g. turning off the Decimal->Decimal mapping with an option). Or should we just advise the explicit type mapping for the users in this case?
>     I would think that using --map-column-hive should be sufficient, correct me if I am wrong here.
>     
>     Thanks

Hi Attila,

Do you have any further updates to this review?

Thanks


- Eric


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57551/#review169453
-----------------------------------------------------------


On March 28, 2017, 4:35 a.m., Eric Lin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57551/
> -----------------------------------------------------------
> 
> (Updated March 28, 2017, 4:35 a.m.)
> 
> 
> Review request for Sqoop, Attila Szabo and Szabolcs Vasas.
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> Currently Sqoop converts DECIMAL from RDMS into DOUBLE in Hive, which is not correct as user will lose precisions. Since Hive supports DECIMAL long ago, we should support DECIMAL to DECIMAL conversion from Sqoop to Hive.
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/hive/HiveTypes.java ad00535 
>   src/java/org/apache/sqoop/hive/TableDefWriter.java 32fcca3 
>   src/test/com/cloudera/sqoop/hive/TestHiveImport.java 33e0cc4 
>   src/test/com/cloudera/sqoop/hive/TestTableDefWriter.java dbf0dde 
>   testdata/hive/scripts/decimalImport.q PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/57551/diff/5/
> 
> 
> Testing
> -------
> 
> Test case + maunaul test
> 
> 
> Thanks,
> 
> Eric Lin
> 
>