You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Jim Scott <ji...@13ways.com> on 2014/09/23 21:52:44 UTC
Review Request 25949: changes to support configuring text formats
(field
separator, record delimiter, quote character, header line, strip quotes)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25949/
-----------------------------------------------------------
Review request for drill.
Repository: drill-git
Description
-------
Updates for handling text formatted files to address: https://issues.apache.org/jira/browse/DRILL-1440
Diffs
-----
exec/java-exec/pom.xml 81dbeff
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java 76c4ace
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java b64a032
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java 7b8761c
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java 31b1fbe
exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/25949/diff/
Testing
-------
There is a test class added to the serialization json work of the data formats including maintaing backward compatibility as well as altering the field separator name.
Maven tests were run, with these two errors:
TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b]
TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: Failure while trying to close allocator: Child level allocators not closed.
I could not figure out the cause for these failures, nor could I properly debug why they were occurring. The code in the patch should have no impact on those tests (although, I could be wrong).
Thanks,
Jim Scott
Re: Review Request 25949: changes to support configuring text formats
(field
separator, record delimiter, quote character, header line, strip quotes)
Posted by Jim Scott <ji...@13ways.com>.
> On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java, line 70
> > <https://reviews.apache.org/r/25949/diff/1/?file=703157#file703157line70>
> >
> > Would that be better to pass the TextFormatConfig instance to DrillTextRecordReader here? We won't update the constructor signature each time we add/remove/rename a property.
An argument could be made either way. The fact that you are asking puts you in the same position as me. I only followed the model that was put in place. I'm personally indifferent and chose not to modify the model that was in place and only add the functionality.
> On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java, line 138
> > <https://reviews.apache.org/r/25949/diff/1/?file=703157#file703157line138>
> >
> > Any particular reason for using dot notation instead of camel casing the JsonProperty names? It would be more consistent with the rest of config options across Drill if camel casing was preferred.
The reason is that I couldn't find a single example of camel casing the json properties. I found plenty of examples in the drill json configuration files that used dots to separate instead of came case.
> On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java, line 140
> > <https://reviews.apache.org/r/25949/diff/1/?file=703157#file703157line140>
> >
> > can we reuse this whole code block for each and every setter that needs this functionality?
You could if you want to. However the rules that are in place don't have to restrict to a single character. I used single character to respect what was already in place. If that changed then it would be potentially custom for each setter.
> On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java, line 219
> > <https://reviews.apache.org/r/25949/diff/1/?file=703157#file703157line219>
> >
> > Though this is not part of the change, did you have a chance to take a look at this? This does not seem right way to compute the hash code.
No I did not. I would consider this issue outside of the scope of this review.
> On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java, line 27
> > <https://reviews.apache.org/r/25949/diff/1/?file=703160#file703160line27>
> >
> > In regards to the failing unit tests. Can we make sure all of the unit test suites are passing?
The point of my comment on this review was to get someone else to validate if what I was seeing was on my machine / in my environment or not.
> On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java, line 25
> > <https://reviews.apache.org/r/25949/diff/1/?file=703160#file703160line25>
> >
> > we should remove @author
If it is against a formatting rule to have an @author tag then it should be a part of the checkstyle rules or documented somewhere. I don't recall having seen that as a requirement to submitting functionality.
- Jim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25949/#review58735
-----------------------------------------------------------
On Sept. 23, 2014, 2:53 p.m., Jim Scott wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25949/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2014, 2:53 p.m.)
>
>
> Review request for drill.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Updates for handling text formatted files to address: https://issues.apache.org/jira/browse/DRILL-1440
>
>
> Diffs
> -----
>
> exec/java-exec/pom.xml 81dbeff
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java 76c4ace
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java b64a032
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java 7b8761c
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java 31b1fbe
> exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25949/diff/
>
>
> Testing
> -------
>
> There is a test class added to the serialization json work of the data formats including maintaing backward compatibility as well as altering the field separator name.
>
> Maven tests were run, with these two errors:
> TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b]
> TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: Failure while trying to close allocator: Child level allocators not closed.
>
> I could not figure out the cause for these failures, nor could I properly debug why they were occurring. The code in the patch should have no impact on those tests (although, I could be wrong).
>
>
> Thanks,
>
> Jim Scott
>
>
Re: Review Request 25949: changes to support configuring text formats
(field
separator, record delimiter, quote character, header line, strip quotes)
Posted by Hanifi Gunes <hg...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25949/#review58735
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java
<https://reviews.apache.org/r/25949/#comment99871>
Would that be better to pass the TextFormatConfig instance to DrillTextRecordReader here? We won't update the constructor signature each time we add/remove/rename a property.
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java
<https://reviews.apache.org/r/25949/#comment99868>
Any particular reason for using dot notation instead of camel casing the JsonProperty names? It would be more consistent with the rest of config options across Drill if camel casing was preferred.
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java
<https://reviews.apache.org/r/25949/#comment99869>
can we reuse this whole code block for each and every setter that needs this functionality?
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java
<https://reviews.apache.org/r/25949/#comment99870>
Though this is not part of the change, did you have a chance to take a look at this? This does not seem right way to compute the hash code.
exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java
<https://reviews.apache.org/r/25949/#comment99894>
we should remove @author
exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java
<https://reviews.apache.org/r/25949/#comment99896>
In regards to the failing unit tests. Can we make sure all of the unit test suites are passing?
- Hanifi Gunes
On Sept. 23, 2014, 7:53 p.m., Jim Scott wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25949/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2014, 7:53 p.m.)
>
>
> Review request for drill.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Updates for handling text formatted files to address: https://issues.apache.org/jira/browse/DRILL-1440
>
>
> Diffs
> -----
>
> exec/java-exec/pom.xml 81dbeff
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java 76c4ace
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java b64a032
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java 7b8761c
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java 31b1fbe
> exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25949/diff/
>
>
> Testing
> -------
>
> There is a test class added to the serialization json work of the data formats including maintaing backward compatibility as well as altering the field separator name.
>
> Maven tests were run, with these two errors:
> TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b]
> TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: Failure while trying to close allocator: Child level allocators not closed.
>
> I could not figure out the cause for these failures, nor could I properly debug why they were occurring. The code in the patch should have no impact on those tests (although, I could be wrong).
>
>
> Thanks,
>
> Jim Scott
>
>
Re: Review Request 25949: changes to support configuring text formats
(field
separator, record delimiter, quote character, header line, strip quotes)
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25949/#review59555
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java
<https://reviews.apache.org/r/25949/#comment100827>
Please review the JSON plans that Drill generates. They use this extensively. Your examples are passthroughs to Hive and HBase config objects. We can't control those. This is why they are inconsistent with the Drill style.
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java
<https://reviews.apache.org/r/25949/#comment100828>
The comment above doesn't discuss anywhere why we use three different types to represent the same value (byte, char and string). Please update to be consistent.
- Jacques Nadeau
On Sept. 23, 2014, 7:53 p.m., Jim Scott wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25949/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2014, 7:53 p.m.)
>
>
> Review request for drill.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Updates for handling text formatted files to address: https://issues.apache.org/jira/browse/DRILL-1440
>
>
> Diffs
> -----
>
> exec/java-exec/pom.xml 81dbeff
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java 76c4ace
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java b64a032
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java 7b8761c
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java 31b1fbe
> exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25949/diff/
>
>
> Testing
> -------
>
> There is a test class added to the serialization json work of the data formats including maintaing backward compatibility as well as altering the field separator name.
>
> Maven tests were run, with these two errors:
> TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b]
> TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: Failure while trying to close allocator: Child level allocators not closed.
>
> I could not figure out the cause for these failures, nor could I properly debug why they were occurring. The code in the patch should have no impact on those tests (although, I could be wrong).
>
>
> Thanks,
>
> Jim Scott
>
>
Re: Review Request 25949: changes to support configuring text formats
(field
separator, record delimiter, quote character, header line, strip quotes)
Posted by Jim Scott <ji...@13ways.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25949/
-----------------------------------------------------------
(Updated Nov. 7, 2014, 11:51 a.m.)
Review request for drill.
Repository: drill-git
Description
-------
Updates for handling text formatted files to address: https://issues.apache.org/jira/browse/DRILL-1440
Diffs
-----
exec/java-exec/pom.xml 81dbeff
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java 76c4ace
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java b64a032
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java 7b8761c
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java 31b1fbe
exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/25949/diff/
Testing
-------
There is a test class added to the serialization json work of the data formats including maintaing backward compatibility as well as altering the field separator name.
Maven tests were run, with these two errors:
TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b]
TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: Failure while trying to close allocator: Child level allocators not closed.
I could not figure out the cause for these failures, nor could I properly debug why they were occurring. The code in the patch should have no impact on those tests (although, I could be wrong).
File Attachments (updated)
----------------
both.patch
https://reviews.apache.org/media/uploaded/files/2014/11/07/21f007f8-e528-44fb-a6df-371aa9f11c23__both.patch
Thanks,
Jim Scott
Re: Review Request 25949: changes to support configuring text formats
(field
separator, record delimiter, quote character, header line, strip quotes)
Posted by Jim Scott <ji...@13ways.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25949/
-----------------------------------------------------------
(Updated Nov. 3, 2014, 6:48 p.m.)
Review request for drill.
Repository: drill-git
Description
-------
Updates for handling text formatted files to address: https://issues.apache.org/jira/browse/DRILL-1440
Diffs
-----
exec/java-exec/pom.xml 81dbeff
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java 76c4ace
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java b64a032
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java 7b8761c
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java 31b1fbe
exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/25949/diff/
Testing
-------
There is a test class added to the serialization json work of the data formats including maintaing backward compatibility as well as altering the field separator name.
Maven tests were run, with these two errors:
TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b]
TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: Failure while trying to close allocator: Child level allocators not closed.
I could not figure out the cause for these failures, nor could I properly debug why they were occurring. The code in the patch should have no impact on those tests (although, I could be wrong).
File Attachments (updated)
----------------
next.patch
https://reviews.apache.org/media/uploaded/files/2014/11/04/4d6a73d2-09d6-4d0d-bf7b-bfad2afab189__next.patch
Thanks,
Jim Scott
Re: Review Request 25949: changes to support configuring text formats
(field
separator, record delimiter, quote character, header line, strip quotes)
Posted by Jim Scott <ji...@13ways.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25949/
-----------------------------------------------------------
(Updated Nov. 3, 2014, 6:38 p.m.)
Review request for drill.
Repository: drill-git
Description
-------
Updates for handling text formatted files to address: https://issues.apache.org/jira/browse/DRILL-1440
Diffs
-----
exec/java-exec/pom.xml 81dbeff
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java 76c4ace
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java b64a032
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java 7b8761c
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java 31b1fbe
exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/25949/diff/
Testing
-------
There is a test class added to the serialization json work of the data formats including maintaing backward compatibility as well as altering the field separator name.
Maven tests were run, with these two errors:
TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b]
TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: Failure while trying to close allocator: Child level allocators not closed.
I could not figure out the cause for these failures, nor could I properly debug why they were occurring. The code in the patch should have no impact on those tests (although, I could be wrong).
File Attachments (updated)
----------------
changes.diff
https://reviews.apache.org/media/uploaded/files/2014/11/04/1e35fb1f-8646-4f10-b47f-647cbf3422c9__changes.diff
Thanks,
Jim Scott
Re: Review Request 25949: changes to support configuring text formats
(field
separator, record delimiter, quote character, header line, strip quotes)
Posted by Jim Scott <ji...@13ways.com>.
> On Nov. 2, 2014, 5:20 p.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java, line 138
> > <https://reviews.apache.org/r/25949/diff/1/?file=703157#file703157line138>
> >
> > we use dashes to describe a multiword. such as field-separator
Please show me an example of this. Dots or no separator at all were all I found. I also didn't find anything in any supporting documentation or guidelines, if they exist I am happy to follow. Here are examples of json objects I referenced. You will see there are no separators for storageformat, dot separators for config settings and camelCase for configProps.
{
"type" : "file",
"enabled" : true,
"connection" : "file:///",
"workspaces" : {
"root" : {
"location" : "/",
"writable" : false,
"storageformat" : null
}
}
hbase.sys.drill
{
"type" : "hbase",
"config" : {
"hbase.zookeeper.quorum" : "localhost",
"hbase.zookeeper.property.clientPort" : "2181"
},
"enabled" : false
}
hive.sys.drill
{
"type" : "hive",
"enabled" : false,
"configProps" : {
"hive.metastore.uris" : "",
"javax.jdo.option.ConnectionURL" : "jdbc:derby:;databaseName=../../sample-data/drill_hive_db;create=true",
"hive.metastore.warehouse.dir" : "/tmp/drill_hive_wh",
"fs.default.name" : "file:///",
"hive.metastore.sasl.enabled" : "false"
}
}
> On Nov. 2, 2014, 5:20 p.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java, line 241
> > <https://reviews.apache.org/r/25949/diff/1/?file=703157#file703157line241>
> >
> > this seems wrong. Shouldn't these be equivalent sets?
This equals method tests this json object:
"real" : {
"type" : "text",
"extensions" : [ "csv" ],
"delimiter" : ","
}
"fake" : {
"type" : "text",
"extensions" : [ "csv" ],
"delimiter" : ","
}
If everything is the same they are equal. If I have both of these definitions they will equal one another regardless of the name "real" / "fake".
> On Nov. 2, 2014, 5:20 p.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java, line 76
> > <https://reviews.apache.org/r/25949/diff/1/?file=703158#file703158line76>
> >
> > why wouldn't you just use a byte in the previous position. You should be consistent.
Not sure I follow. Do you mean change the constructor to require bytes? If so I was just following the pattern put in place.
> On Nov. 2, 2014, 5:20 p.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java, line 116
> > <https://reviews.apache.org/r/25949/diff/1/?file=703158#file703158line116>
> >
> > Why are you jumping back to string here? Why drop to char/byte and then come back?
In this code base we utilize the byte. In the hadoop core library it uses a string. I have a comment explaining that right above that line of code.
- Jim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25949/#review59545
-----------------------------------------------------------
On Sept. 23, 2014, 2:53 p.m., Jim Scott wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25949/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2014, 2:53 p.m.)
>
>
> Review request for drill.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Updates for handling text formatted files to address: https://issues.apache.org/jira/browse/DRILL-1440
>
>
> Diffs
> -----
>
> exec/java-exec/pom.xml 81dbeff
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java 76c4ace
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java b64a032
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java 7b8761c
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java 31b1fbe
> exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25949/diff/
>
>
> Testing
> -------
>
> There is a test class added to the serialization json work of the data formats including maintaing backward compatibility as well as altering the field separator name.
>
> Maven tests were run, with these two errors:
> TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b]
> TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: Failure while trying to close allocator: Child level allocators not closed.
>
> I could not figure out the cause for these failures, nor could I properly debug why they were occurring. The code in the patch should have no impact on those tests (although, I could be wrong).
>
>
> Thanks,
>
> Jim Scott
>
>
Re: Review Request 25949: changes to support configuring text formats
(field
separator, record delimiter, quote character, header line, strip quotes)
Posted by Jim Scott <ji...@13ways.com>.
> On Nov. 2, 2014, 5:20 p.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java, line 116
> > <https://reviews.apache.org/r/25949/diff/1/?file=703158#file703158line116>
> >
> > Why are you jumping back to string here? Why drop to char/byte and then come back?
>
> Jim Scott wrote:
> In this code base we utilize the byte. In the hadoop core library it uses a string. I have a comment explaining that right above that line of code.
This is the only case where it is used as a string. All other uses of this are as byte.
- Jim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25949/#review59545
-----------------------------------------------------------
On Sept. 23, 2014, 2:53 p.m., Jim Scott wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25949/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2014, 2:53 p.m.)
>
>
> Review request for drill.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Updates for handling text formatted files to address: https://issues.apache.org/jira/browse/DRILL-1440
>
>
> Diffs
> -----
>
> exec/java-exec/pom.xml 81dbeff
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java 76c4ace
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java b64a032
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java 7b8761c
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java 31b1fbe
> exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25949/diff/
>
>
> Testing
> -------
>
> There is a test class added to the serialization json work of the data formats including maintaing backward compatibility as well as altering the field separator name.
>
> Maven tests were run, with these two errors:
> TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b]
> TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: Failure while trying to close allocator: Child level allocators not closed.
>
> I could not figure out the cause for these failures, nor could I properly debug why they were occurring. The code in the patch should have no impact on those tests (although, I could be wrong).
>
>
> Thanks,
>
> Jim Scott
>
>
Re: Review Request 25949: changes to support configuring text formats
(field
separator, record delimiter, quote character, header line, strip quotes)
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25949/#review59545
-----------------------------------------------------------
Your note at the top about failing tests. Did the master commit before your commit work? Otherwise, small fixes. Try to get this fixed tomorrow so we can get this in early this week.
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java
<https://reviews.apache.org/r/25949/#comment100803>
Please just clean this up so you aren't doing a cast over and over again. The other issue is fine as is.
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java
<https://reviews.apache.org/r/25949/#comment100804>
we use dashes to describe a multiword. such as field-separator
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java
<https://reviews.apache.org/r/25949/#comment100805>
Yes, please refactor this out as a utility method. E.g. getCharacter(String value, String error message, char defaultValue) and similar if there are variations.
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java
<https://reviews.apache.org/r/25949/#comment100808>
remove comment.
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java
<https://reviews.apache.org/r/25949/#comment100809>
this seems wrong. Shouldn't these be equivalent sets?
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java
<https://reviews.apache.org/r/25949/#comment100806>
why wouldn't you just use a byte in the previous position. You should be consistent.
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java
<https://reviews.apache.org/r/25949/#comment100807>
Why are you jumping back to string here? Why drop to char/byte and then come back?
exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java
<https://reviews.apache.org/r/25949/#comment100811>
Yes, it is against the rules. Same for all Apache products. Feel free to open a separate JIRA to add checkstyle rules and submit a patch.
exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java
<https://reviews.apache.org/r/25949/#comment100813>
Once you get your updates done, please request this on the mailing list and someone will run the suite to verify that your system is borked. In general, you should assume that the test suite runs to completion.
- Jacques Nadeau
On Sept. 23, 2014, 7:53 p.m., Jim Scott wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25949/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2014, 7:53 p.m.)
>
>
> Review request for drill.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Updates for handling text formatted files to address: https://issues.apache.org/jira/browse/DRILL-1440
>
>
> Diffs
> -----
>
> exec/java-exec/pom.xml 81dbeff
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java 76c4ace
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java b64a032
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java 7b8761c
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java 31b1fbe
> exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25949/diff/
>
>
> Testing
> -------
>
> There is a test class added to the serialization json work of the data formats including maintaing backward compatibility as well as altering the field separator name.
>
> Maven tests were run, with these two errors:
> TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b]
> TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: Failure while trying to close allocator: Child level allocators not closed.
>
> I could not figure out the cause for these failures, nor could I properly debug why they were occurring. The code in the patch should have no impact on those tests (although, I could be wrong).
>
>
> Thanks,
>
> Jim Scott
>
>
Re: Review Request 25949: changes to support configuring text formats
(field
separator, record delimiter, quote character, header line, strip quotes)
Posted by Jim Scott <ji...@13ways.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25949/
-----------------------------------------------------------
(Updated Sept. 23, 2014, 7:53 p.m.)
Review request for drill.
Repository: drill-git
Description
-------
Updates for handling text formatted files to address: https://issues.apache.org/jira/browse/DRILL-1440
Diffs
-----
exec/java-exec/pom.xml 81dbeff
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java 76c4ace
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java b64a032
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java 7b8761c
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java 31b1fbe
exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/25949/diff/
Testing
-------
There is a test class added to the serialization json work of the data formats including maintaing backward compatibility as well as altering the field separator name.
Maven tests were run, with these two errors:
TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b]
TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: Failure while trying to close allocator: Child level allocators not closed.
I could not figure out the cause for these failures, nor could I properly debug why they were occurring. The code in the patch should have no impact on those tests (although, I could be wrong).
Thanks,
Jim Scott