> 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 > >
