close function instead of mutable reference

[liyongjing]

Describe the bug
A clear and concise description of what the bug is.

use super::builder;
use arrow::record_batch::RecordBatch;
use parquet::{
    arrow::ArrowWriter,
    basic::Compression,
    file::properties::{WriterProperties, WriterVersion},
};
use std::{fs::File, path::Path};

pub struct Writer {
    writer: ArrowWriter<File>,
}

impl Writer {
    pub fn new<P: AsRef<Path>>(path: P) -> Self {
        let schema = builder::get_schema();
        let file = File::create(&path).unwrap();
        let props = WriterProperties::builder().build();
        let writer = ArrowWriter::try_new(file, schema, Some(props)).unwrap();
        Self { writer }
    }

    pub fn write(&mut self, batch: &RecordBatch) {
        self.writer.write(batch).expect("Writing batch");
    }

    pub fn close(&mut self) {
        // writer must be closed to write footer
        self.writer.close().unwrap();
    }
}

To Reproduce
Steps to reproduce the behavior:

   |         self.writer.close().unwrap();
   |         ^^^^^^^^^^^ move occurs because `self.writer` has type `ArrowWriter<std::fs::File>`, which does not implement the `Copy` trait

Expected behavior
A clear and concise description of what you expected to happen.

write and close use the same parameters

parquet-17.0.0/src/arrow/arrow_writer/mod.rs 222 line

    /// Close and finalize the underlying Parquet writer
    pub fn close(&mut self) -> Result<parquet_format::FileMetaData> {
        self.flush()?;
        self.writer.close()
    }

parquet-17.0.0/src/file/writer.rs 167 line

    pub fn close(&mut self) -> Result<parquet::FileMetaData> {
        self.assert_previous_writer_closed()?;
        let metadata = self.write_metadata()?;
        Ok(metadata)
    }

Additional context
Add any other context about the problem here.

[tustvold]

This was an intentional change, to truncate the lifetime of the writer on calling close. See this thread for more context #1719 (comment)

The change is more obviously beneficial on the ColumnWriter, etc... but you can also see it on ArrowWriter. Consider

let file = File::create(&path).unwrap();
let writer = ArrowWriter::try_new(&mut file, ...);

...

writer.close();

file.rewind().unwrap();

Without this change you need to manually drop the writer in order to reuse the file. We also benefit from the borrow checker making sure we can't call close multiple times.

If you are integrating with some system that requires &mut self for some reason, Option::take should allow you to get the previous semantics.

[liyongjing]

I call write multiple times, call close one times using static HashMap.

fn global() -> &'static Mutex<HashMap<String, Writer>> {
    static INSTANCE: OnceCell<Mutex<HashMap<String, Writer>>> = OnceCell::new();
    INSTANCE.get_or_init(|| Mutex::new(HashMap::new()))
}
pub fn write(path: &str, val: Vec<u8>) {
    let mut g = global_channel().lock().unwrap();
    let w = g.entry(path.to_string()).or_insert(Writer::new(path));
    let batch =  ...;
    w.write(&batch);
}
pub fn close(path: &str) {
    let mut g = global_channel().lock().unwrap();
    if let Some(v) = g.get_mut(path) {
        // *v don't copy
        v.close();
    }
    g.remove(path);
}

[tustvold]

How about?

pub fn close(path: &str) -> Result<()> {
    let mut g = global_channel().lock().unwrap()
    if let Some(v) = g.remove(path) {
        v.close()?;
    }
}

[liyongjing]

thanks for you comment