You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/05/17 23:34:39 UTC
[GitHub] [arrow] houqp opened a new pull request #7210: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
houqp opened a new pull request #7210:
URL: https://github.com/apache/arrow/pull/7210
This PR changes schema argument for scan_csv method into `Option<&Schema>`. Other related changes are needed to make this happen including:
* added delimiter argument to all csv related structs and functions
* fixed a bug in schema field inference function
* made `arrow::csv::reader::infer_file_schema` public so it can be used by data fusion
Known limitations:
* when provided with a directory of csv files, schema inference code only reads rows from the first file.
* to avoid adding yet another argument to all csv related functions, i hard coded number of rows to read for schema inference to 100
Open questions:
* Should we rename `datasource::csv::CsvFile` struct to `CsvTable` to keep it consistent with ParquetTable and MemoryTable? The implementation of CsvFile also supports reading from a directory of files, so `CsvFile` is not an accurate name.
* csv related function arguments are getting a bit long, should we introduce a csv option struct to capture the following configs with sensible defaults?
- schema
- has_header
- delimiter
- infer_max_read_records
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove closed pull request #7210: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #7210:
URL: https://github.com/apache/arrow/pull/7210
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #7210: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #7210:
URL: https://github.com/apache/arrow/pull/7210#discussion_r427655895
##########
File path: rust/datafusion/src/execution/physical_plan/csv.rs
##########
@@ -71,15 +75,35 @@ impl CsvExec {
/// Create a new execution plan for reading a set of CSV files
pub fn try_new(
path: &str,
- schema: Arc<Schema>,
+ schema: Option<Arc<Schema>>,
has_header: bool,
+ delimiter: Option<u8>,
projection: Option<Vec<usize>>,
batch_size: usize,
) -> Result<Self> {
+ let schema = match schema {
+ Some(s) => s,
+ None => {
+ let mut filenames: Vec<String> = vec![];
+ common::build_file_list(path, &mut filenames, ".csv")?;
+ if filenames.is_empty() {
+ return Err(ExecutionError::General("No files found".to_string()));
+ }
+
+ let f = File::open(&filenames[0])?;
Review comment:
Yes, we could definitely make this more sophisticated (as a follow up PR). I think this is great for examples and getting started though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #7210: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #7210:
URL: https://github.com/apache/arrow/pull/7210#discussion_r427607065
##########
File path: rust/datafusion/src/execution/physical_plan/csv.rs
##########
@@ -71,15 +75,35 @@ impl CsvExec {
/// Create a new execution plan for reading a set of CSV files
pub fn try_new(
path: &str,
- schema: Arc<Schema>,
+ schema: Option<Arc<Schema>>,
has_header: bool,
+ delimiter: Option<u8>,
projection: Option<Vec<usize>>,
batch_size: usize,
) -> Result<Self> {
+ let schema = match schema {
+ Some(s) => s,
+ None => {
+ let mut filenames: Vec<String> = vec![];
+ common::build_file_list(path, &mut filenames, ".csv")?;
+ if filenames.is_empty() {
+ return Err(ExecutionError::General("No files found".to_string()));
+ }
+
+ let f = File::open(&filenames[0])?;
Review comment:
I hear your argument, maybe let's wait for Andy's opinion first
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on a change in pull request #7210: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #7210:
URL: https://github.com/apache/arrow/pull/7210#discussion_r427605959
##########
File path: rust/arrow/src/csv/reader.rs
##########
@@ -87,19 +87,19 @@ fn infer_field_schema(string: &str) -> DataType {
/// with `max_read_records` controlling the maximum number of records to read.
///
/// If `max_read_records` is not set, the whole file is read to infer its schema.
-fn infer_file_schema<R: Read + Seek>(
+pub fn infer_file_schema<R: Read + Seek>(
reader: &mut BufReader<R>,
delimiter: u8,
max_read_records: Option<usize>,
- has_headers: bool,
Review comment:
yeah, i have been thinking about that as well, @andygrove what do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #7210: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #7210:
URL: https://github.com/apache/arrow/pull/7210#discussion_r427600229
##########
File path: rust/datafusion/src/execution/physical_plan/csv.rs
##########
@@ -71,15 +75,35 @@ impl CsvExec {
/// Create a new execution plan for reading a set of CSV files
pub fn try_new(
path: &str,
- schema: Arc<Schema>,
+ schema: Option<Arc<Schema>>,
has_header: bool,
+ delimiter: Option<u8>,
projection: Option<Vec<usize>>,
batch_size: usize,
) -> Result<Self> {
+ let schema = match schema {
+ Some(s) => s,
+ None => {
+ let mut filenames: Vec<String> = vec![];
+ common::build_file_list(path, &mut filenames, ".csv")?;
+ if filenames.is_empty() {
+ return Err(ExecutionError::General("No files found".to_string()));
+ }
+
+ let f = File::open(&filenames[0])?;
Review comment:
What would happen if the first file is empty, but other files aren't? A good follow-up could be iterating through the files until an `Option<max_inference: usize>` is reached to ensure that the desired sample size is reached.
I've personally encountered some CSV files where the first k records conformed to a schema, but things broke down thereafter, especially with files that aren't generated by a uniformity-preserving tool like Spark or a database.
##########
File path: rust/arrow/src/csv/reader.rs
##########
@@ -762,10 +778,20 @@ mod tests {
let mut csv = builder.build(file).unwrap();
match csv.next() {
Err(e) => assert_eq!(
- "ParseError(\"Error while parsing value 4.x4 at line 4\")",
+ "ParseError(\"Error while parsing value 4.x4 for column 1 at line 4\")",
format!("{:?}", e)
),
Ok(_) => panic!("should have failed"),
}
}
+
+ #[test]
+ fn test_infer_field_schema() {
Review comment:
Thanks for adding this :)
##########
File path: rust/datafusion/src/execution/physical_plan/csv.rs
##########
@@ -71,15 +75,35 @@ impl CsvExec {
/// Create a new execution plan for reading a set of CSV files
pub fn try_new(
path: &str,
- schema: Arc<Schema>,
+ schema: Option<Arc<Schema>>,
has_header: bool,
+ delimiter: Option<u8>,
projection: Option<Vec<usize>>,
batch_size: usize,
) -> Result<Self> {
+ let schema = match schema {
+ Some(s) => s,
+ None => {
+ let mut filenames: Vec<String> = vec![];
+ common::build_file_list(path, &mut filenames, ".csv")?;
+ if filenames.is_empty() {
+ return Err(ExecutionError::General("No files found".to_string()));
+ }
+
+ let f = File::open(&filenames[0])?;
+ Arc::new(csv::infer_file_schema(
+ &mut BufReader::new(f),
+ delimiter.unwrap_or(b','),
+ Some(30),
Review comment:
Using a number that's too small for inference could cause issues for some users (esp as it's not yet configurable). How about 1000?
##########
File path: rust/arrow/src/csv/reader.rs
##########
@@ -87,19 +87,19 @@ fn infer_field_schema(string: &str) -> DataType {
/// with `max_read_records` controlling the maximum number of records to read.
///
/// If `max_read_records` is not set, the whole file is read to infer its schema.
-fn infer_file_schema<R: Read + Seek>(
+pub fn infer_file_schema<R: Read + Seek>(
reader: &mut BufReader<R>,
delimiter: u8,
max_read_records: Option<usize>,
- has_headers: bool,
Review comment:
Should we change `has_headers` to `has_header` consistently on the rest of the `csv` submodule?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7210: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7210:
URL: https://github.com/apache/arrow/pull/7210#issuecomment-629880393
https://issues.apache.org/jira/browse/ARROW-8839
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on a change in pull request #7210: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #7210:
URL: https://github.com/apache/arrow/pull/7210#discussion_r427605427
##########
File path: rust/datafusion/src/execution/physical_plan/csv.rs
##########
@@ -71,15 +75,35 @@ impl CsvExec {
/// Create a new execution plan for reading a set of CSV files
pub fn try_new(
path: &str,
- schema: Arc<Schema>,
+ schema: Option<Arc<Schema>>,
has_header: bool,
+ delimiter: Option<u8>,
projection: Option<Vec<usize>>,
batch_size: usize,
) -> Result<Self> {
+ let schema = match schema {
+ Some(s) => s,
+ None => {
+ let mut filenames: Vec<String> = vec![];
+ common::build_file_list(path, &mut filenames, ".csv")?;
+ if filenames.is_empty() {
+ return Err(ExecutionError::General("No files found".to_string()));
+ }
+
+ let f = File::open(&filenames[0])?;
Review comment:
yeah, there is no guarantee no matter what we do unless we read all the entries. Even with max_inference, we can't guarantee the remaining rows will confirm to the inferred schema.
the way i look at this is manually specify a schema if you want correctness and performance. only use schema inference if you just want to get a quick and dirty query up and running.
that said, i will try to change it to read all files instead of first one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #7210: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #7210:
URL: https://github.com/apache/arrow/pull/7210#discussion_r427655072
##########
File path: rust/arrow/src/csv/reader.rs
##########
@@ -87,19 +87,19 @@ fn infer_field_schema(string: &str) -> DataType {
/// with `max_read_records` controlling the maximum number of records to read.
///
/// If `max_read_records` is not set, the whole file is read to infer its schema.
-fn infer_file_schema<R: Read + Seek>(
+pub fn infer_file_schema<R: Read + Seek>(
reader: &mut BufReader<R>,
delimiter: u8,
max_read_records: Option<usize>,
- has_headers: bool,
Review comment:
That would be great.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7210: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7210:
URL: https://github.com/apache/arrow/pull/7210#issuecomment-631089381
+1 to creating a struct for csv options
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on pull request #7210: WIP: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #7210:
URL: https://github.com/apache/arrow/pull/7210#issuecomment-631143176
all feedback addressed, I will send separate PRs to add CSV read option struct and improve inference file read behavior to keep this one simple.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on a change in pull request #7210: ARROW-8839: [Rust] [DataFusion] support CSV schema inference in logical plan
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #7210:
URL: https://github.com/apache/arrow/pull/7210#discussion_r427602640
##########
File path: rust/datafusion/src/execution/physical_plan/csv.rs
##########
@@ -71,15 +75,35 @@ impl CsvExec {
/// Create a new execution plan for reading a set of CSV files
pub fn try_new(
path: &str,
- schema: Arc<Schema>,
+ schema: Option<Arc<Schema>>,
has_header: bool,
+ delimiter: Option<u8>,
projection: Option<Vec<usize>>,
batch_size: usize,
) -> Result<Self> {
+ let schema = match schema {
+ Some(s) => s,
+ None => {
+ let mut filenames: Vec<String> = vec![];
+ common::build_file_list(path, &mut filenames, ".csv")?;
+ if filenames.is_empty() {
+ return Err(ExecutionError::General("No files found".to_string()));
+ }
+
+ let f = File::open(&filenames[0])?;
+ Arc::new(csv::infer_file_schema(
+ &mut BufReader::new(f),
+ delimiter.unwrap_or(b','),
+ Some(30),
Review comment:
good catch, will change to 1000, that 30 was a number i used for debugging purpose, forgot to change it back :P
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org