I hope it's ok to ask for some feedback here. Of not, please let me know. The rules did not sound like against it
I'm very new to Rust. And while in general my coding background is ok, Rust still feels alien to me. I think I'm just still at "how to think in Rust" part of the curve.
So I would like to ask here for opinions on the following bit of code. I know that those unwrap are too optimistic for production, and I could figure out how to pass io::Error from the function all the way up to the shell. But what are other choices that I don't see?
What would you write differently? What looks like Pythonisms/C++isms? Or is missing the mark completely?
use std::{fs, io};
use std::path::PathBuf;
use std::convert::TryFrom;
use clap::Parser;
use parquet::file::reader::SerializedFileReader;
use parquet::record;
use csv::WriterBuilder;
#[derive(Debug, Parser)]
#[command(version, about, long_about = None)]
struct Args {
dir: String,
#[arg(default_value = "0")]
count: usize
}
fn get_files_in_dir(dir: &str) -> Option<Vec<PathBuf>>
{
let dir = fs::read_dir(dir);
if dir.is_err() {
return None
};
let files = dir.unwrap()
.map(|res| res.map(|e| e.path()))
.collect::<Result<Vec<_>, _>>();
if files.is_err() {
return None
}
files.ok()
}
fn read_parquet_dir(entries: &Vec<String>) -> impl Iterator<Item = record::Row> {
entries.iter()
.map(|p| SerializedFileReader::try_from(p.clone()).unwrap())
.flat_map(|r| r.into_iter())
.map(|r| r.unwrap())
}
fn main() -> Result<(), io::Error> {
let args = Args::parse();
let entries = match get_files_in_dir(&args.dir)
{
Some(entries) => entries,
None => return Ok(())
};
let mut wtr = WriterBuilder::new().from_writer(io::stdout());
for (idx, row) in read_parquet_dir(&entries.iter().map(|p| p.display().to_string()).collect()).enumerate() {
let values: Vec<String> = row.get_column_iter().map(|(_column, value)| value.to_string()).collect();
if idx == 0 {
wtr.serialize(row.get_column_iter().map(|(column, _value)| column.to_string()).collect::<Vec<String>>())?;
}
wtr.serialize(values)?;
if args.count>0 && idx+1 == args.count {
break;
}
}
Ok(())
}
7 Comments
BB_C@programming.dev · 7 pts · 200d
Let's do this incrementally, shall we?
First, let's
make get_files_in_dir()idiomatic. We will get back to errors later.Now, in
read_parquet_dir(), if the unwraps stem from confidence that we will never get errors, then we can confidently ignore them (we will get back to the errors later).Now, let's go back to
get_files_in_dir(), and not ignore errors.Now,
SerializedFileReader::try_from()is implemented for&Path, andPathBufderefs to&Path. So your dance of converting to display then to string (which is lossy btw) is not needed.While we're at it, let's use a slice instead of
&Vec<_>in the signature (clippy would tell you about this if you have it set up with rust-analyzer).Now let's see what we can do about not ignoring errors in
read_parquet_dir().Approach 1: Save intermediate reader results
This consumes all readers before getting further. So, it's a behavioral change. The signature may also scare some people 😉
Approach 2: Wrapper iterator type
How can we combine errors from readers with flat record results?
This is how.
Approach 3 (bonus): Using unstable
#![feature(gen_blocks)]INeedMana@piefed.zip · 2 pts · 200d
Oh wow! Thank you very much for such a deep dive
So
try_from(&**p)is not a code smell/poor form in Rust?BB_C@programming.dev · 4 pts · 200d
No. It's how you (explicitly) go from ref to deref.
Here:
pis&PathBuf*pisPathBuf**pisPath(Deref)&**pis&Path.Since what you started with is a reference to a non-Copy value, you can't do anything that would use/move
*por**p. Furthermore,Pathis an unsized type (just likestrand[T]), so you need to reference it (or Box it) in any case.Another way to do this is:
Some APIs use
AsRefin signatures to allow passing references of different types directly (e.g. File::open()), but that doesn't apply here.NGram@piefed.ca · 5 pts · 200d
Your usage and implementation of
get_files_in_diris quite awkward. It is returningNoneon errors and then you're returningOk(())when it returnsNone, ignoring a bunch of errors. Why not just haveget_files_in_dirreturn aResult<Vec<PathBuf>, io::Error>? You could drastically simplify the function by just?ing the results.INeedMana@piefed.zip · 1 pts · 200d
Yes, thank you. Mainly, I've noticed this opportunity after I cleaned up the code from all the previous back and forth. I decided to leave it as it was to get an opinion on that. Also, I clearly had wrong understanding about how real Crustaceans use
?shape_warrior_t@programming.dev · 4 pts · 200d
Not very familiar with the libraries, and BB_C has already given a lot of more detailed feedback, but here are some small things:
get_files_in_diras a single chain of method calls, you can still replace with In general, if you can useif letorlet elseto pattern match, prefer that over unwrapping -- Clippy has a lint relating to this, actually.return Noneinget_files_in_dir.INeedMana@piefed.zip · 3 pts · 200d
Thank you very much! TIL about Clippy :D