You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Qian Xu <qi...@intel.com> on 2015/04/21 04:51:59 UTC

Review Request 33387: SQOOP-2294: Change to Avro schema name breaks some use cases

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

Review request for Sqoop.


Bugs: SQOOP-2294
    https://issues.apache.org/jira/browse/SQOOP-2294


Repository: sqoop-trunk


Description
-------

SQOOP-1395 introduced a breaking change that causes issues for some existing users. The patch intends to do two things:
1. For `--as-avrodatafile`, Sqoop will behaviors same as it does in 1.4.5.
2. For `--as-parquetfile`, Sqoop rename the temporary generated java class and set Avro schema name to table name (same as what `--as-avrodatafile` does)


Diffs
-----

  src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java e70d23c 
  src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java ed8e8b1 
  src/java/org/apache/sqoop/tool/CodeGenTool.java 6bd7f1d 

Diff: https://reviews.apache.org/r/33387/diff/


Testing
-------

All passed


Thanks,

Qian Xu


Re: Review Request 33387: SQOOP-2294: Change to Avro schema name breaks some use cases

Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33387/#review80970
-----------------------------------------------------------

Ship it!


Ship It!

- Jarek Cecho


On April 21, 2015, 2:51 a.m., Qian Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33387/
> -----------------------------------------------------------
> 
> (Updated April 21, 2015, 2:51 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-2294
>     https://issues.apache.org/jira/browse/SQOOP-2294
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> SQOOP-1395 introduced a breaking change that causes issues for some existing users. The patch intends to do two things:
> 1. For `--as-avrodatafile`, Sqoop will behaviors same as it does in 1.4.5.
> 2. For `--as-parquetfile`, Sqoop rename the temporary generated java class and set Avro schema name to table name (same as what `--as-avrodatafile` does)
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java e70d23c 
>   src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java ed8e8b1 
>   src/java/org/apache/sqoop/tool/CodeGenTool.java 6bd7f1d 
> 
> Diff: https://reviews.apache.org/r/33387/diff/
> 
> 
> Testing
> -------
> 
> All passed
> 
> 
> Thanks,
> 
> Qian Xu
> 
>


Re: Review Request 33387: SQOOP-2294: Change to Avro schema name breaks some use cases

Posted by Qian Xu <qi...@intel.com>.

> On April 22, 2015, 1:06 a.m., Keegan Witt wrote:
> > I think this solution will work, but possibly it could be more cleanly implemented. Could
> > `String avroName = schemaNameOverride != null ? schemaNameOverride : (shortClassName == null ? avroTableName : shortClassName);`
> > instead be
> > `String avroName = shortClassName == null ? avroTableName : shortClassName;`
> > and in `DataDrivenImportJob` use `options.setClassName()` to prefix with "codegen_" (if Parquet) rather than passing `schemaNameOverride`?
> > Then you could also remove the conditional logic in `CodeGenTool`.
> > 
> > Or am I misunderstanding here?

The java class was generated already, before `DataDrivenImportJob` is executed. I have not found any single place modification for that. If we can tell Kite SDK not to check schema defination against same name java class on class path, we will not have this issue. I'll ask Kite SDK to see, if we can make it more straightforward in future release.


- Qian


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


On April 21, 2015, 10:51 a.m., Qian Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33387/
> -----------------------------------------------------------
> 
> (Updated April 21, 2015, 10:51 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-2294
>     https://issues.apache.org/jira/browse/SQOOP-2294
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> SQOOP-1395 introduced a breaking change that causes issues for some existing users. The patch intends to do two things:
> 1. For `--as-avrodatafile`, Sqoop will behaviors same as it does in 1.4.5.
> 2. For `--as-parquetfile`, Sqoop rename the temporary generated java class and set Avro schema name to table name (same as what `--as-avrodatafile` does)
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java e70d23c 
>   src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java ed8e8b1 
>   src/java/org/apache/sqoop/tool/CodeGenTool.java 6bd7f1d 
> 
> Diff: https://reviews.apache.org/r/33387/diff/
> 
> 
> Testing
> -------
> 
> All passed
> 
> 
> Thanks,
> 
> Qian Xu
> 
>


Re: Review Request 33387: SQOOP-2294: Change to Avro schema name breaks some use cases

Posted by Keegan Witt <ke...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33387/#review80997
-----------------------------------------------------------


I think this solution will work, but possibly it could be more cleanly implemented. Could
`String avroName = schemaNameOverride != null ? schemaNameOverride : (shortClassName == null ? avroTableName : shortClassName);`
instead be
`String avroName = shortClassName == null ? avroTableName : shortClassName;`
and in `DataDrivenImportJob` use `options.setClassName()` to prefix with "codegen_" (if Parquet) rather than passing `schemaNameOverride`?
Then you could also remove the conditional logic in `CodeGenTool`.

Or am I misunderstanding here?

- Keegan Witt


On April 20, 2015, 10:51 p.m., Qian Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33387/
> -----------------------------------------------------------
> 
> (Updated April 20, 2015, 10:51 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-2294
>     https://issues.apache.org/jira/browse/SQOOP-2294
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> SQOOP-1395 introduced a breaking change that causes issues for some existing users. The patch intends to do two things:
> 1. For `--as-avrodatafile`, Sqoop will behaviors same as it does in 1.4.5.
> 2. For `--as-parquetfile`, Sqoop rename the temporary generated java class and set Avro schema name to table name (same as what `--as-avrodatafile` does)
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java e70d23c 
>   src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java ed8e8b1 
>   src/java/org/apache/sqoop/tool/CodeGenTool.java 6bd7f1d 
> 
> Diff: https://reviews.apache.org/r/33387/diff/
> 
> 
> Testing
> -------
> 
> All passed
> 
> 
> Thanks,
> 
> Qian Xu
> 
>