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/11/29 21:40:29 UTC

Review Request 28537: Sqoop2:Support Map Type in CSV IDF

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

Review request for Sqoop.


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


Repository: sqoop-sqoop2


Description
-------

see jira


Diffs
-----

  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 

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


Testing
-------

yes


Thanks,

Veena Basavaraj


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28537/
-----------------------------------------------------------

(Updated Dec. 1, 2014, 12:05 p.m.)


Review request for Sqoop.


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


Repository: sqoop-sqoop2


Description
-------

see jira


Diffs (updated)
-----

  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 

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


Testing
-------

yes


Thanks,

Veena Basavaraj


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Veena Basavaraj <vb...@cloudera.com>.

> On Nov. 30, 2014, 6:28 a.m., Jarek Cecho wrote:
> > Other then the unnecessary re-order of import statement, the patch looks good to me.
> 
> Jarek Cecho wrote:
>     Please attach final patch to the JIRA and I'll commit it.

i have already uploaded the patch, and to make it more obvious it is not an unnecessary order, I just defined the order that we should be using in all files going forward, and that is 

com
org
java
javax


- Veena


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


On Nov. 29, 2014, 3:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 3:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Jarek Cecho <ja...@apache.org>.

> On Nov. 30, 2014, 2:28 p.m., Jarek Cecho wrote:
> > Other then the unnecessary re-order of import statement, the patch looks good to me.

Please attach final patch to the JIRA and I'll commit it.


- Jarek


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


On Nov. 29, 2014, 11:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 11:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

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

Ship it!


Other then the unnecessary re-order of import statement, the patch looks good to me.

- Jarek Cecho


On Nov. 29, 2014, 11:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 11:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28537/
-----------------------------------------------------------

(Updated Nov. 29, 2014, 3:09 p.m.)


Review request for Sqoop.


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


Repository: sqoop-sqoop2


Description
-------

see jira


Diffs (updated)
-----

  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 

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


Testing
-------

yes


Thanks,

Veena Basavaraj


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Veena Basavaraj <vb...@cloudera.com>.

> On Nov. 29, 2014, 1:57 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, line 607
> > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line607>
> >
> >     Since the Map's value is Array of Text, I would expect that we put here Array of Strings, rather then one String that represents serialized Array.
> 
> Veena Basavaraj wrote:
>     schema.addColumn(new org.apache.sqoop.schema.type.Map("1", new Text("key"), new Array("value", new FixedPoint("number"))));

fixed, all caught another issue while fixing this, added a test case for it.


- Veena


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


On Nov. 29, 2014, 3:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 3:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Jarek Cecho <ja...@apache.org>.

> On Nov. 29, 2014, 9:57 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, lines 21-52
> > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line21>
> >
> >     Can we please not rearange imports as part of this patch?
> 
> Veena Basavaraj wrote:
>     Let me highlight one thing.
>     
>     there needs to be a standard we follow in all files.
>     
>     
>     
>     In the CSVIntermediateDataFormat. the order is:
>     
>     com
>     org
>     java
>     javax
>     I put this in the "IDE" fomratter.
>     
>     the test file has some other random order. we cannot have this, it is super in productive, if you want this to continue, lets stop been anal about such a trivial thing.
>     
>     My 2 cents, it would be good for the project to have a standardized order when I import new things, I use a shortcut in IDE and let the formatter take care of it.
>     
>     I dont manually type in every new import, that is not my good use of time.
> 
> Jarek Cecho wrote:
>     You're correct that we are missing proper guidelines on how the import should be ordered and I'm all for to define that. We should however have a discussion about that in separate JIRA, not part of a different unrelated JIRA.
>     
>     I'm wondering how is this making you a less productive? I'm not typing each import manually myself, I'm using my IDE (InteliJ) to do that for me. All I'm asking to not rearange the imports automatically.
> 
> Veena Basavaraj wrote:
>     If you read my comment above carefully, I have even told you to the order I use in the IDE, based on the CSVIntermediateDataFormat class, and this thing seems to be custom order in each file.
>     
>     What is your issue with sticking to one order that is in CSVIntermediateDataFormat
>     
>     com
>     org
>     java
>     javax
>     
>     Really you think spending time on a RB on import prder is the best use of your time and my time? An IDE has one set of rules. When I do CLTR+SHIFT+O, I have not seen an IDE that has rules per file yet,

I don't have a problem with defining import order in the project. I however believe that such change should be discussed openly in standalone JIRA rather then on Review board as a part of unrelated change.

I'm not familiar with CTLR+SHIFT+O shortcut. Doing little bit of googling it seems that it's an Eclipse specific shortcut to re-organize imports. Let me know if I'm incorrect here. If that is right, then until we will have set of rules to follow, it seems to me that we can simply not call this shortcut? If all that the shortcut does is to reorganize the imports, then I'm assuming it won't have any impact on productivity?

The reason why we are trying not to have unrelated changes in patches (such as re-organizing imports, ...) is that doing unrelated changes makes difficult to use commands such as "git blame" and "git cherrypick" that we are using very often.


- Jarek


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


On Nov. 29, 2014, 11:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 11:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Veena Basavaraj <vb...@cloudera.com>.

> On Nov. 29, 2014, 1:57 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, lines 21-52
> > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line21>
> >
> >     Can we please not rearange imports as part of this patch?
> 
> Veena Basavaraj wrote:
>     Let me highlight one thing.
>     
>     there needs to be a standard we follow in all files.
>     
>     
>     
>     In the CSVIntermediateDataFormat. the order is:
>     
>     com
>     org
>     java
>     javax
>     I put this in the "IDE" fomratter.
>     
>     the test file has some other random order. we cannot have this, it is super in productive, if you want this to continue, lets stop been anal about such a trivial thing.
>     
>     My 2 cents, it would be good for the project to have a standardized order when I import new things, I use a shortcut in IDE and let the formatter take care of it.
>     
>     I dont manually type in every new import, that is not my good use of time.
> 
> Jarek Cecho wrote:
>     You're correct that we are missing proper guidelines on how the import should be ordered and I'm all for to define that. We should however have a discussion about that in separate JIRA, not part of a different unrelated JIRA.
>     
>     I'm wondering how is this making you a less productive? I'm not typing each import manually myself, I'm using my IDE (InteliJ) to do that for me. All I'm asking to not rearange the imports automatically.

If you read my comment above carefully, I have even told you to the order I use in the IDE, based on the CSVIntermediateDataFormat class, and this thing seems to be custom order in each file.

What is your issue with sticking to one order that is in CSVIntermediateDataFormat

com
org
java
javax

Really you think spending time on a RB on import prder is the best use of your time and my time? An IDE has one set of rules. When I do CLTR+SHIFT+O, I have not seen an IDE that has rules per file yet,


- Veena


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


On Nov. 29, 2014, 3:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 3:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Jarek Cecho <ja...@apache.org>.

> On Nov. 29, 2014, 9:57 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, lines 21-52
> > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line21>
> >
> >     Can we please not rearange imports as part of this patch?
> 
> Veena Basavaraj wrote:
>     Let me highlight one thing.
>     
>     there needs to be a standard we follow in all files.
>     
>     
>     
>     In the CSVIntermediateDataFormat. the order is:
>     
>     com
>     org
>     java
>     javax
>     I put this in the "IDE" fomratter.
>     
>     the test file has some other random order. we cannot have this, it is super in productive, if you want this to continue, lets stop been anal about such a trivial thing.
>     
>     My 2 cents, it would be good for the project to have a standardized order when I import new things, I use a shortcut in IDE and let the formatter take care of it.
>     
>     I dont manually type in every new import, that is not my good use of time.

You're correct that we are missing proper guidelines on how the import should be ordered and I'm all for to define that. We should however have a discussion about that in separate JIRA, not part of a different unrelated JIRA.

I'm wondering how is this making you a less productive? I'm not typing each import manually myself, I'm using my IDE (InteliJ) to do that for me. All I'm asking to not rearange the imports automatically.


- Jarek


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


On Nov. 29, 2014, 11:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 11:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Jarek Cecho <ja...@apache.org>.

> On Nov. 29, 2014, 9:57 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, lines 21-52
> > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line21>
> >
> >     Can we please not rearange imports as part of this patch?
> 
> Veena Basavaraj wrote:
>     Let me highlight one thing.
>     
>     there needs to be a standard we follow in all files.
>     
>     
>     
>     In the CSVIntermediateDataFormat. the order is:
>     
>     com
>     org
>     java
>     javax
>     I put this in the "IDE" fomratter.
>     
>     the test file has some other random order. we cannot have this, it is super in productive, if you want this to continue, lets stop been anal about such a trivial thing.
>     
>     My 2 cents, it would be good for the project to have a standardized order when I import new things, I use a shortcut in IDE and let the formatter take care of it.
>     
>     I dont manually type in every new import, that is not my good use of time.
> 
> Jarek Cecho wrote:
>     You're correct that we are missing proper guidelines on how the import should be ordered and I'm all for to define that. We should however have a discussion about that in separate JIRA, not part of a different unrelated JIRA.
>     
>     I'm wondering how is this making you a less productive? I'm not typing each import manually myself, I'm using my IDE (InteliJ) to do that for me. All I'm asking to not rearange the imports automatically.
> 
> Veena Basavaraj wrote:
>     If you read my comment above carefully, I have even told you to the order I use in the IDE, based on the CSVIntermediateDataFormat class, and this thing seems to be custom order in each file.
>     
>     What is your issue with sticking to one order that is in CSVIntermediateDataFormat
>     
>     com
>     org
>     java
>     javax
>     
>     Really you think spending time on a RB on import prder is the best use of your time and my time? An IDE has one set of rules. When I do CLTR+SHIFT+O, I have not seen an IDE that has rules per file yet,
> 
> Jarek Cecho wrote:
>     I don't have a problem with defining import order in the project. I however believe that such change should be discussed openly in standalone JIRA rather then on Review board as a part of unrelated change.
>     
>     I'm not familiar with CTLR+SHIFT+O shortcut. Doing little bit of googling it seems that it's an Eclipse specific shortcut to re-organize imports. Let me know if I'm incorrect here. If that is right, then until we will have set of rules to follow, it seems to me that we can simply not call this shortcut? If all that the shortcut does is to reorganize the imports, then I'm assuming it won't have any impact on productivity?
>     
>     The reason why we are trying not to have unrelated changes in patches (such as re-organizing imports, ...) is that doing unrelated changes makes difficult to use commands such as "git blame" and "git cherrypick" that we are using very often.
> 
> Veena Basavaraj wrote:
>     not calling the shortcut means I manually type in every import.
>     
>     It adds the imports that are missing as well. 
>     
>     
>     kafka project for instance does not really worry about the import order.
>     
>     I have spent time to create a wiki on style guide and sent the email to dev@. High time we had this. I vote for google style guide that is standard across many projects. If you have others in mind, leave a feedback there.
>     
>     If we enforce a standard and there is a import re-ordering this means that it is part of the change and doing a git blame should be self-explanatory.
> 
> Veena Basavaraj wrote:
>     As far as this patch is concerned, I am ok to put this on hold until there is resolution. 
>     
>     If everyone in the sqoop dev list votes for not having a standard and then assumes that we will keep a random order in every file just to avoid any more changes, I want to see this discussion in the dev@ list.

Thank you for putting together the code guidelines. I'll definitely read them and follow up there.

I don't se a need to hold this patch though. Can we simply drop the re-arrangement and commit the patch?


- Jarek


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


On Nov. 29, 2014, 11:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 11:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Veena Basavaraj <vb...@cloudera.com>.

> On Nov. 29, 2014, 1:57 p.m., Jarek Cecho wrote:
> > I have one major question that is highlighled below in the Test. It seems that we are not creating proper object representation of trully nested data types, as the "Map<String, Array<String>> will expect (and return) Map<String, String>. I'm wondering if this is intentional or not?

good point, addressed this in the test cases.


- Veena


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


On Nov. 29, 2014, 3:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 3:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Veena Basavaraj <vb...@cloudera.com>.

> On Nov. 29, 2014, 1:57 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, line 607
> > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line607>
> >
> >     Since the Map's value is Array of Text, I would expect that we put here Array of Strings, rather then one String that represents serialized Array.

schema.addColumn(new org.apache.sqoop.schema.type.Map("1", new Text("key"), new Array("value", new FixedPoint("number"))));


> On Nov. 29, 2014, 1:57 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, lines 21-52
> > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line21>
> >
> >     Can we please not rearange imports as part of this patch?

Let me highlight one thing.

there needs to be a standard we follow in all files.


In the CSVIntermediateDataFormat. the order is:

com
org
java
javax
I put this in the "IDE" fomratter.

the test file has some other random order. we cannot have this, it is super in productive, if you want this to continue, lets stop been anal about such a trivial thing.

My 2 cents, it would be good for the project to have a standardized order when I import new things, I use a shortcut in IDE and let the formatter take care of it.

I dont manually type in every new import, that is not my good use of time.


- Veena


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


On Nov. 29, 2014, 12:40 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 12:40 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Veena Basavaraj <vb...@cloudera.com>.

> On Nov. 29, 2014, 1:57 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, lines 21-52
> > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line21>
> >
> >     Can we please not rearange imports as part of this patch?
> 
> Veena Basavaraj wrote:
>     Let me highlight one thing.
>     
>     there needs to be a standard we follow in all files.
>     
>     
>     
>     In the CSVIntermediateDataFormat. the order is:
>     
>     com
>     org
>     java
>     javax
>     I put this in the "IDE" fomratter.
>     
>     the test file has some other random order. we cannot have this, it is super in productive, if you want this to continue, lets stop been anal about such a trivial thing.
>     
>     My 2 cents, it would be good for the project to have a standardized order when I import new things, I use a shortcut in IDE and let the formatter take care of it.
>     
>     I dont manually type in every new import, that is not my good use of time.
> 
> Jarek Cecho wrote:
>     You're correct that we are missing proper guidelines on how the import should be ordered and I'm all for to define that. We should however have a discussion about that in separate JIRA, not part of a different unrelated JIRA.
>     
>     I'm wondering how is this making you a less productive? I'm not typing each import manually myself, I'm using my IDE (InteliJ) to do that for me. All I'm asking to not rearange the imports automatically.
> 
> Veena Basavaraj wrote:
>     If you read my comment above carefully, I have even told you to the order I use in the IDE, based on the CSVIntermediateDataFormat class, and this thing seems to be custom order in each file.
>     
>     What is your issue with sticking to one order that is in CSVIntermediateDataFormat
>     
>     com
>     org
>     java
>     javax
>     
>     Really you think spending time on a RB on import prder is the best use of your time and my time? An IDE has one set of rules. When I do CLTR+SHIFT+O, I have not seen an IDE that has rules per file yet,
> 
> Jarek Cecho wrote:
>     I don't have a problem with defining import order in the project. I however believe that such change should be discussed openly in standalone JIRA rather then on Review board as a part of unrelated change.
>     
>     I'm not familiar with CTLR+SHIFT+O shortcut. Doing little bit of googling it seems that it's an Eclipse specific shortcut to re-organize imports. Let me know if I'm incorrect here. If that is right, then until we will have set of rules to follow, it seems to me that we can simply not call this shortcut? If all that the shortcut does is to reorganize the imports, then I'm assuming it won't have any impact on productivity?
>     
>     The reason why we are trying not to have unrelated changes in patches (such as re-organizing imports, ...) is that doing unrelated changes makes difficult to use commands such as "git blame" and "git cherrypick" that we are using very often.

not calling the shortcut means I manually type in every import.

It adds the imports that are missing as well. 


kafka project for instance does not really worry about the import order.

I have spent time to create a wiki on style guide and sent the email to dev@. High time we had this. I vote for google style guide that is standard across many projects. If you have others in mind, leave a feedback there.

If we enforce a standard and there is a import re-ordering this means that it is part of the change and doing a git blame should be self-explanatory.


- Veena


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


On Nov. 29, 2014, 3:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 3:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Veena Basavaraj <vb...@cloudera.com>.

> On Nov. 29, 2014, 1:57 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, lines 21-52
> > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line21>
> >
> >     Can we please not rearange imports as part of this patch?
> 
> Veena Basavaraj wrote:
>     Let me highlight one thing.
>     
>     there needs to be a standard we follow in all files.
>     
>     
>     
>     In the CSVIntermediateDataFormat. the order is:
>     
>     com
>     org
>     java
>     javax
>     I put this in the "IDE" fomratter.
>     
>     the test file has some other random order. we cannot have this, it is super in productive, if you want this to continue, lets stop been anal about such a trivial thing.
>     
>     My 2 cents, it would be good for the project to have a standardized order when I import new things, I use a shortcut in IDE and let the formatter take care of it.
>     
>     I dont manually type in every new import, that is not my good use of time.
> 
> Jarek Cecho wrote:
>     You're correct that we are missing proper guidelines on how the import should be ordered and I'm all for to define that. We should however have a discussion about that in separate JIRA, not part of a different unrelated JIRA.
>     
>     I'm wondering how is this making you a less productive? I'm not typing each import manually myself, I'm using my IDE (InteliJ) to do that for me. All I'm asking to not rearange the imports automatically.
> 
> Veena Basavaraj wrote:
>     If you read my comment above carefully, I have even told you to the order I use in the IDE, based on the CSVIntermediateDataFormat class, and this thing seems to be custom order in each file.
>     
>     What is your issue with sticking to one order that is in CSVIntermediateDataFormat
>     
>     com
>     org
>     java
>     javax
>     
>     Really you think spending time on a RB on import prder is the best use of your time and my time? An IDE has one set of rules. When I do CLTR+SHIFT+O, I have not seen an IDE that has rules per file yet,
> 
> Jarek Cecho wrote:
>     I don't have a problem with defining import order in the project. I however believe that such change should be discussed openly in standalone JIRA rather then on Review board as a part of unrelated change.
>     
>     I'm not familiar with CTLR+SHIFT+O shortcut. Doing little bit of googling it seems that it's an Eclipse specific shortcut to re-organize imports. Let me know if I'm incorrect here. If that is right, then until we will have set of rules to follow, it seems to me that we can simply not call this shortcut? If all that the shortcut does is to reorganize the imports, then I'm assuming it won't have any impact on productivity?
>     
>     The reason why we are trying not to have unrelated changes in patches (such as re-organizing imports, ...) is that doing unrelated changes makes difficult to use commands such as "git blame" and "git cherrypick" that we are using very often.
> 
> Veena Basavaraj wrote:
>     not calling the shortcut means I manually type in every import.
>     
>     It adds the imports that are missing as well. 
>     
>     
>     kafka project for instance does not really worry about the import order.
>     
>     I have spent time to create a wiki on style guide and sent the email to dev@. High time we had this. I vote for google style guide that is standard across many projects. If you have others in mind, leave a feedback there.
>     
>     If we enforce a standard and there is a import re-ordering this means that it is part of the change and doing a git blame should be self-explanatory.
> 
> Veena Basavaraj wrote:
>     As far as this patch is concerned, I am ok to put this on hold until there is resolution. 
>     
>     If everyone in the sqoop dev list votes for not having a standard and then assumes that we will keep a random order in every file just to avoid any more changes, I want to see this discussion in the dev@ list.
> 
> Jarek Cecho wrote:
>     Thank you for putting together the code guidelines. I'll definitely read them and follow up there.
>     
>     I don't se a need to hold this patch though. Can we simply drop the re-arrangement and commit the patch?

for this patch I will, but any patch going forward, it is best we have a resolution.

lets take a vote on the following, I will send an email to dev@

1. We do not care about the order, so lets not bother if reorder occurs, there are projects that follow this ( kafka)
2. We do care, lets impose the rule going forward on any file we touch
3. We do care a lot, lets fix every file
4. We do care, but lets not modify any existing file, but only for new files, if a developer by mistake does it, lets not frown upon it, just point them to the wiki.


- Veena


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


On Nov. 29, 2014, 3:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 3:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Veena Basavaraj <vb...@cloudera.com>.

> On Nov. 29, 2014, 1:57 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, lines 21-52
> > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line21>
> >
> >     Can we please not rearange imports as part of this patch?
> 
> Veena Basavaraj wrote:
>     Let me highlight one thing.
>     
>     there needs to be a standard we follow in all files.
>     
>     
>     
>     In the CSVIntermediateDataFormat. the order is:
>     
>     com
>     org
>     java
>     javax
>     I put this in the "IDE" fomratter.
>     
>     the test file has some other random order. we cannot have this, it is super in productive, if you want this to continue, lets stop been anal about such a trivial thing.
>     
>     My 2 cents, it would be good for the project to have a standardized order when I import new things, I use a shortcut in IDE and let the formatter take care of it.
>     
>     I dont manually type in every new import, that is not my good use of time.
> 
> Jarek Cecho wrote:
>     You're correct that we are missing proper guidelines on how the import should be ordered and I'm all for to define that. We should however have a discussion about that in separate JIRA, not part of a different unrelated JIRA.
>     
>     I'm wondering how is this making you a less productive? I'm not typing each import manually myself, I'm using my IDE (InteliJ) to do that for me. All I'm asking to not rearange the imports automatically.
> 
> Veena Basavaraj wrote:
>     If you read my comment above carefully, I have even told you to the order I use in the IDE, based on the CSVIntermediateDataFormat class, and this thing seems to be custom order in each file.
>     
>     What is your issue with sticking to one order that is in CSVIntermediateDataFormat
>     
>     com
>     org
>     java
>     javax
>     
>     Really you think spending time on a RB on import prder is the best use of your time and my time? An IDE has one set of rules. When I do CLTR+SHIFT+O, I have not seen an IDE that has rules per file yet,
> 
> Jarek Cecho wrote:
>     I don't have a problem with defining import order in the project. I however believe that such change should be discussed openly in standalone JIRA rather then on Review board as a part of unrelated change.
>     
>     I'm not familiar with CTLR+SHIFT+O shortcut. Doing little bit of googling it seems that it's an Eclipse specific shortcut to re-organize imports. Let me know if I'm incorrect here. If that is right, then until we will have set of rules to follow, it seems to me that we can simply not call this shortcut? If all that the shortcut does is to reorganize the imports, then I'm assuming it won't have any impact on productivity?
>     
>     The reason why we are trying not to have unrelated changes in patches (such as re-organizing imports, ...) is that doing unrelated changes makes difficult to use commands such as "git blame" and "git cherrypick" that we are using very often.
> 
> Veena Basavaraj wrote:
>     not calling the shortcut means I manually type in every import.
>     
>     It adds the imports that are missing as well. 
>     
>     
>     kafka project for instance does not really worry about the import order.
>     
>     I have spent time to create a wiki on style guide and sent the email to dev@. High time we had this. I vote for google style guide that is standard across many projects. If you have others in mind, leave a feedback there.
>     
>     If we enforce a standard and there is a import re-ordering this means that it is part of the change and doing a git blame should be self-explanatory.

As far as this patch is concerned, I am ok to put this on hold until there is resolution. 

If everyone in the sqoop dev list votes for not having a standard and then assumes that we will keep a random order in every file just to avoid any more changes, I want to see this discussion in the dev@ list.


- Veena


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


On Nov. 29, 2014, 3:09 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 3:09 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

Posted by Jarek Cecho <ja...@apache.org>.

> On Nov. 29, 2014, 9:57 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, lines 21-52
> > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line21>
> >
> >     Can we please not rearange imports as part of this patch?
> 
> Veena Basavaraj wrote:
>     Let me highlight one thing.
>     
>     there needs to be a standard we follow in all files.
>     
>     
>     
>     In the CSVIntermediateDataFormat. the order is:
>     
>     com
>     org
>     java
>     javax
>     I put this in the "IDE" fomratter.
>     
>     the test file has some other random order. we cannot have this, it is super in productive, if you want this to continue, lets stop been anal about such a trivial thing.
>     
>     My 2 cents, it would be good for the project to have a standardized order when I import new things, I use a shortcut in IDE and let the formatter take care of it.
>     
>     I dont manually type in every new import, that is not my good use of time.
> 
> Jarek Cecho wrote:
>     You're correct that we are missing proper guidelines on how the import should be ordered and I'm all for to define that. We should however have a discussion about that in separate JIRA, not part of a different unrelated JIRA.
>     
>     I'm wondering how is this making you a less productive? I'm not typing each import manually myself, I'm using my IDE (InteliJ) to do that for me. All I'm asking to not rearange the imports automatically.
> 
> Veena Basavaraj wrote:
>     If you read my comment above carefully, I have even told you to the order I use in the IDE, based on the CSVIntermediateDataFormat class, and this thing seems to be custom order in each file.
>     
>     What is your issue with sticking to one order that is in CSVIntermediateDataFormat
>     
>     com
>     org
>     java
>     javax
>     
>     Really you think spending time on a RB on import prder is the best use of your time and my time? An IDE has one set of rules. When I do CLTR+SHIFT+O, I have not seen an IDE that has rules per file yet,
> 
> Jarek Cecho wrote:
>     I don't have a problem with defining import order in the project. I however believe that such change should be discussed openly in standalone JIRA rather then on Review board as a part of unrelated change.
>     
>     I'm not familiar with CTLR+SHIFT+O shortcut. Doing little bit of googling it seems that it's an Eclipse specific shortcut to re-organize imports. Let me know if I'm incorrect here. If that is right, then until we will have set of rules to follow, it seems to me that we can simply not call this shortcut? If all that the shortcut does is to reorganize the imports, then I'm assuming it won't have any impact on productivity?
>     
>     The reason why we are trying not to have unrelated changes in patches (such as re-organizing imports, ...) is that doing unrelated changes makes difficult to use commands such as "git blame" and "git cherrypick" that we are using very often.
> 
> Veena Basavaraj wrote:
>     not calling the shortcut means I manually type in every import.
>     
>     It adds the imports that are missing as well. 
>     
>     
>     kafka project for instance does not really worry about the import order.
>     
>     I have spent time to create a wiki on style guide and sent the email to dev@. High time we had this. I vote for google style guide that is standard across many projects. If you have others in mind, leave a feedback there.
>     
>     If we enforce a standard and there is a import re-ordering this means that it is part of the change and doing a git blame should be self-explanatory.
> 
> Veena Basavaraj wrote:
>     As far as this patch is concerned, I am ok to put this on hold until there is resolution. 
>     
>     If everyone in the sqoop dev list votes for not having a standard and then assumes that we will keep a random order in every file just to avoid any more changes, I want to see this discussion in the dev@ list.
> 
> Jarek Cecho wrote:
>     Thank you for putting together the code guidelines. I'll definitely read them and follow up there.
>     
>     I don't se a need to hold this patch though. Can we simply drop the re-arrangement and commit the patch?
> 
> Veena Basavaraj wrote:
>     for this patch I will, but any patch going forward, it is best we have a resolution.
>     
>     lets take a vote on the following, I will send an email to dev@
>     
>     1. We do not care about the order, so lets not bother if reorder occurs, there are projects that follow this ( kafka)
>     2. We do care, lets impose the rule going forward on any file we touch
>     3. We do care a lot, lets fix every file
>     4. We do care, but lets not modify any existing file, but only for new files, if a developer by mistake does it, lets not frown upon it, just point them to the wiki.

Thank you for updating the patch. I'll run tests and commit if everything passes.

Let's take the formating discussion to the email thread at dev@, so that everyone have ability to jump in.


- Jarek


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


On Dec. 1, 2014, 8:05 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Dec. 1, 2014, 8:05 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Re: Review Request 28537: Sqoop2:Support Map Type in CSV IDF

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


I have one major question that is highlighled below in the Test. It seems that we are not creating proper object representation of trully nested data types, as the "Map<String, Array<String>> will expect (and return) Map<String, String>. I'm wondering if this is intentional or not?


connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java
<https://reviews.apache.org/r/28537/#comment105457>

    Can we please not rearange imports as part of this patch?



connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java
<https://reviews.apache.org/r/28537/#comment105458>

    Since the Map's value is Array of Text, I would expect that we put here Array of Strings, rather then one String that represents serialized Array.


Jarcec

- Jarek Cecho


On Nov. 29, 2014, 8:40 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28537/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2014, 8:40 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1750
>     https://issues.apache.org/jira/browse/SQOOP-1750
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 4f2baf9 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java b629897 
> 
> Diff: https://reviews.apache.org/r/28537/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>