> On Feb. 7, 2015, 3:30 p.m., Veena Basavaraj wrote: > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsExtractor.java, > > line 119 > > <https://reviews.apache.org/r/30303/diff/4/?file=853438#file853438line119> > > > > I would prefer we keep these methods in CSVIDFUtils. > > > > SqoopIDFUtils was named so, because it handles the IDF column types. > > > > If we are extracting away the functionality of CSVIDF to be used in > > other connectors. then it is best to move thsoe methods into its own utils > > so connectors can use it and have correponding tests for it. there.
Ultimately it would be nice to have our own IDF. Let's consider this a temporary step in the right direction and create an IDF after the release? > On Feb. 7, 2015, 3:30 p.m., Veena Basavaraj wrote: > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsExtractor.java, > > line 188 > > <https://reviews.apache.org/r/30303/diff/4/?file=853438#file853438line188> > > > > this seems such a overkill, whoveever intends to use the object format. > > > > We convert it back and forth between csv to OBJECT and then object to > > CSV and then the same dance hapens on the TO side. > > > > Is this going to be a common use case anyone will use? Just wondnering. > > > > The whole point of using text format in a connector and then use CSVIDF > > was to make sure we have very little conversions going on, but this use > > case seems to just completely do the opposite. > > > > Now if I imagine supporting a avro/ parquet in this conenctor in > > future, I wonder what format it will use. food for thought. HDFS connector should get its own IDF in the future. > On Feb. 7, 2015, 3:30 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/common/SqoopIDFUtils.java, > > line 565 > > <https://reviews.apache.org/r/30303/diff/4/?file=853446#file853446line565> > > > > As comments above, lets not bloat the SQoopIDFUtils, please move the > > CSVIDF methods to its own class, since it is doing more than just the > > common column types we support A couple of thoughts on this: 1. I think it belongs in SqoopIDFUtils because most IDFs will need to go back and forth between CSV and Object form. CSV needs to be implemented by all IDFs. 2. If we want to change this, let's move it out in a different Jira. ``parseCSVString`` was originally part of SqoopIDFUtils any ways. https://github.com/apache/sqoop/blob/f7efa38005525e2fb22ce1af0323bf265d719c00/connector/connector-sdk/src/main/java/org/apache/sqoop/connector/common/SqoopIDFUtils.java#L518 > On Feb. 7, 2015, 3:30 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/common/SqoopIDFUtils.java, > > line 688 > > <https://reviews.apache.org/r/30303/diff/4/?file=853446#file853446line688> > > > > why do we need to move this to IDFUtils, if this is speciifc to CSV, > > then its better to create a SQOOPCSVUtils and not bloat the IDF Utils. IDF > > Utils has methods to handle each column type and hence it was named so. See above comment. > On Feb. 7, 2015, 3:30 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/common/SqoopIDFUtils.java, > > line 445 > > <https://reviews.apache.org/r/30303/diff/4/?file=853446#file853446line445> > > > > there is some new lines added >>>> that shows up in RB I think those are because review board has trouble discerning between method blocks and code blocks? - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30303/#review71553 ----------------------------------------------------------- On Feb. 7, 2015, 2:23 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30303/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2015, 2:23 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1579 > https://issues.apache.org/jira/browse/SQOOP-1579 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > A question for the reviewer: > # Does the option "unescape" with default to "true" make sense? It did for > me, but I can imagine from the users perspective that it might be confusing. > > commit 74274f1d745215035eb766645b9e8dbb4c591139 > Author: Abraham Elmahrek <[email protected]> > Date: Mon Jan 26 19:21:15 2015 -0800 > > SQOOP-1579: Sqoop2: Data transfer to load into Hive does not work > > :100644 100644 9f3367b... 6b8ae52... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsExtractor.java > :100644 100644 556c112... 9dba157... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsLoader.java > :100644 100644 353c1f2... 941c378... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsUtils.java > :100644 100644 b7a9c3d... aa5f316... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/configuration/ToJobConfig.java > :100644 100644 2dca634... 3efc76a... M > connector/connector-hdfs/src/main/resources/hdfs-connector-config.properties > :100644 100644 9a5ec5e... c3b2975... M > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestExtractor.java > :100644 100644 8eefe05... c8d404b... M > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestHdfsUtils.java > :100644 100644 b5ec6da... 62d5eb8... M > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestLoader.java > :100644 100644 c5eb6a3... c2dd812... M > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/common/SqoopIDFUtils.java > :100644 100644 3350599... c92bf61... M > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/AVROIntermediateDataFormat.java > :100644 100644 b716e79... 71d9534... M > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java > :100644 100644 4c67c1a... a8d186d... M > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/JSONIntermediateDataFormat.java > :100644 100644 588ce29... 1166496... M > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/common/TestSqoopIDFUtils.java > :100644 100644 f658d09... a5be42b... M > execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMatching.java > :000000 100644 0000000... 17148bc... A > test/src/main/java/org/apache/sqoop/test/data/ShortStories.java > :100644 100644 1124cd3... 12bf99c... M > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > :100644 100644 a65d862... f6f7188... M > test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java > :100644 100644 f82abc7... a81b349... M > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromHDFSToRDBMSTest.java > :100644 100644 aa9f212... bb91a83... M > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java > > > Diffs > ----- > > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsExtractor.java > f70d56b > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsLoader.java > bdee878 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsUtils.java > 353c1f2 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/configuration/ToJobConfig.java > b7a9c3d > > connector/connector-hdfs/src/main/resources/hdfs-connector-config.properties > 2dca634 > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestExtractor.java > 9a5ec5e > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestHdfsUtils.java > 8eefe05 > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestLoader.java > b5ec6da > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/common/SqoopIDFUtils.java > 71641ed > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java > c233fb2 > > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/common/TestSqoopIDFUtils.java > 588ce29 > test/src/main/java/org/apache/sqoop/test/data/ShortStories.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > 398a051 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromHDFSToRDBMSTest.java > 397ce6f > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java > ced52cc > > Diff: https://reviews.apache.org/r/30303/diff/ > > > Testing > ------- > > mvn clean integration-test > > > Thanks, > > Abraham Elmahrek > >
