Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: arrow_path_to_parquet #1239

Merged
merged 10 commits into from
Mar 24, 2025
86 changes: 71 additions & 15 deletions src/parseable/streams.rs
Original file line number Diff line number Diff line change
@@ -67,6 +67,18 @@ use super::{
LogStream, ARROW_FILE_EXTENSION,
};

/// Returns the filename for parquet if provided arrows file path is valid as per our expectation
fn arrow_path_to_parquet(path: &Path, random_string: &str) -> Option<PathBuf> {
let filename = path.file_stem()?.to_str()?;
let (_, front) = filename.split_once('.')?;
assert!(front.contains('.'), "contains the delim `.`");
let filename_with_random_number = format!("{front}.{random_string}.parquet");
let mut parquet_path = path.to_owned();
parquet_path.set_file_name(filename_with_random_number);

Some(parquet_path)
}
Comment on lines +70 to +80
Copy link

@coderabbitai coderabbitai bot Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Format string doesn't match expected parquet file format

The format string used for constructing the parquet filename on line 75 doesn't include the 'data' portion that's present in the arrow filename, but the tests expect it to be included.

Update the format string to maintain the same structure:

fn arrow_path_to_parquet(path: &Path, random_string: &str) -> Option<PathBuf> {
    let filename = path.file_stem()?.to_str()?;
    let (_, front) = filename.split_once('.')?;
    assert!(front.contains('.'), "contains the delim `.`");
-    let filename_with_random_number = format!("{front}.{random_string}.parquet");
+    let filename_with_random_number = format!("{front}.data.{random_string}.parquet");
    let mut parquet_path = path.to_owned();
    parquet_path.set_file_name(filename_with_random_number);

    Some(parquet_path)
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// Returns the filename for parquet if provided arrows file path is valid as per our expectation
fn arrow_path_to_parquet(path: &Path, random_string: &str) -> Option<PathBuf> {
let filename = path.file_stem()?.to_str()?;
let (_, front) = filename.split_once('.')?;
assert!(front.contains('.'), "contains the delim `.`");
let filename_with_random_number = format!("{front}.{random_string}.parquet");
let mut parquet_path = path.to_owned();
parquet_path.set_file_name(filename_with_random_number);
Some(parquet_path)
}
/// Returns the filename for parquet if provided arrows file path is valid as per our expectation
fn arrow_path_to_parquet(path: &Path, random_string: &str) -> Option<PathBuf> {
let filename = path.file_stem()?.to_str()?;
let (_, front) = filename.split_once('.')?;
assert!(front.contains('.'), "contains the delim `.`");
let filename_with_random_number = format!("{front}.data.{random_string}.parquet");
let mut parquet_path = path.to_owned();
parquet_path.set_file_name(filename_with_random_number);
Some(parquet_path)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

front already carries ".data" at the end as asserted above

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@de-sh You're absolutely right. After tracing through the function with examples from the tests, I can see that "front" already contains ".data" at the end.

For example, when processing a file named "abc.date=2023-10-01.hour=12.minute=30.hostname.data.arrows":

  1. file_stem → "abc.date=2023-10-01.hour=12.minute=30.hostname.data"
  2. After split_once('.') → front = "date=2023-10-01.hour=12.minute=30.hostname.data"
  3. Result with format → "date=2023-10-01.hour=12.minute=30.hostname.data.{random_string}.parquet"

The original implementation is correct. Thanks for the clarification!


✏️ Learnings added
Learnt from: de-sh
PR: parseablehq/parseable#1239
File: src/parseable/streams.rs:70-80
Timestamp: 2025-03-24T06:08:47.139Z
Learning: In the Parseable codebase, arrow filenames follow a structure ending with ".data.arrows". When converting to parquet, the "front" part (after the first dot) already includes ".data" at the end, so there's no need to add it explicitly in the arrow_path_to_parquet function.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


#[derive(Debug, thiserror::Error)]
#[error("Stream not found: {0}")]
pub struct StreamNotFound(pub String);
@@ -182,7 +194,10 @@ impl Stream {
let paths = dir
.flatten()
.map(|file| file.path())
.filter(|file| file.extension().is_some_and(|ext| ext.eq("arrows")))
.filter(|file| {
file.extension()
.is_some_and(|ext| ext.eq(ARROW_FILE_EXTENSION))
})
.sorted_by_key(|f| f.metadata().unwrap().modified().unwrap())
.collect();

@@ -225,12 +240,13 @@ impl Stream {
&arrow_file_path, self.stream_name
);
remove_file(&arrow_file_path).unwrap();
} else {
let key = Self::arrow_path_to_parquet(&arrow_file_path, &random_string);
} else if let Some(key) = arrow_path_to_parquet(&arrow_file_path, &random_string) {
grouped_arrow_file
.entry(key)
.or_default()
.push(arrow_file_path);
} else {
warn!("Unexpected arrows file: {}", arrow_file_path.display());
}
}
grouped_arrow_file
@@ -289,17 +305,6 @@ impl Stream {
}
}

fn arrow_path_to_parquet(path: &Path, random_string: &str) -> PathBuf {
let filename = path.file_stem().unwrap().to_str().unwrap();
let (_, filename) = filename.split_once('.').unwrap();
assert!(filename.contains('.'), "contains the delim `.`");
let filename_with_random_number = format!("{filename}.{random_string}.arrows");
let mut parquet_path = path.to_owned();
parquet_path.set_file_name(filename_with_random_number);
parquet_path.set_extension("parquet");
parquet_path
}

/// Converts arrow files in staging into parquet files, does so only for past minutes when run with `!shutdown_signal`
pub fn prepare_parquet(&self, shutdown_signal: bool) -> Result<(), StagingError> {
info!(
@@ -831,7 +836,7 @@ impl Streams {

#[cfg(test)]
mod tests {
use std::{sync::Barrier, thread::spawn, time::Duration};
use std::{io::Write, sync::Barrier, thread::spawn, time::Duration};

use arrow_array::{Int32Array, StringArray, TimestampMillisecondArray};
use arrow_schema::{DataType, Field, TimeUnit};
@@ -1207,6 +1212,57 @@ mod tests {
assert_eq!(staging.arrow_files().len(), 1);
}

fn create_test_file(dir: &TempDir, filename: &str) -> PathBuf {
let file_path = dir.path().join(filename);
let mut file = File::create(&file_path).expect("Failed to create test file");
// Write some dummy content
file.write_all(b"test content")
.expect("Failed to write to test file");
file_path
}

#[test]
fn test_valid_arrow_path_conversion() {
let temp_dir = TempDir::new().expect("Failed to create temp dir");
let filename = "12345abcde&key1=value1.date=2020-01-21.hour=10.minute=30.key1=value1.key2=value2.ee529ffc8e76.data.arrows";
let file_path = create_test_file(&temp_dir, filename);
let random_string = "random123";

let result = arrow_path_to_parquet(&file_path, random_string);

assert!(result.is_some());
let parquet_path = result.unwrap();
assert_eq!(
parquet_path.file_name().unwrap().to_str().unwrap(),
"date=2020-01-21.hour=10.minute=30.key1=value1.key2=value2.ee529ffc8e76.data.random123.parquet"
);
}
Comment on lines +1224 to +1239
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Test doesn't check filename structure expectation properly

The expected output filename in test_valid_arrow_path_conversion is inconsistent with the current implementation of arrow_path_to_parquet. The test assumes a format that includes the "data" component which would be created by format!("{front}.data.{random_string}.parquet"), but the implementation uses format!("{front}.{random_string}.parquet").

Either update the test expectation or change the implementation to match. If you change the implementation, be sure to update line 75 as suggested above.


#[test]
fn test_complex_path() {
let temp_dir = TempDir::new().expect("Failed to create temp dir");
let nested_dir = temp_dir.path().join("nested/directory/structure");
std::fs::create_dir_all(&nested_dir).expect("Failed to create nested directories");

let filename = "20200201T1830f8a5fc1edc567d56&key1=value1&key2=value2.date=2020-01-21.hour=10.minute=30.region=us-west.ee529ffc8e76.data.arrows";
let file_path = nested_dir.join(filename);

let mut file = File::create(&file_path).expect("Failed to create test file");
file.write_all(b"test content")
.expect("Failed to write to test file");

let random_string = "random456";

let result = arrow_path_to_parquet(&file_path, random_string);

assert!(result.is_some());
let parquet_path = result.unwrap();
assert_eq!(
parquet_path.file_name().unwrap().to_str().unwrap(),
"date=2020-01-21.hour=10.minute=30.region=us-west.ee529ffc8e76.data.random456.parquet"
);
}
Comment on lines +1224 to +1264
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add negative test cases for arrow_path_to_parquet function

While you've added tests for valid paths, there are no tests for invalid paths where the function should return None.

Add tests for cases where the function should return None:

#[test]
fn test_invalid_arrow_path_without_extension() {
    let temp_dir = TempDir::new().expect("Failed to create temp dir");
    let filename = "invalid_filename_without_extension";
    let file_path = create_test_file(&temp_dir, filename);
    let random_string = "random123";

    let result = arrow_path_to_parquet(&file_path, random_string);
    assert!(result.is_none());
}

#[test]
fn test_invalid_arrow_path_without_delimiters() {
    let temp_dir = TempDir::new().expect("Failed to create temp dir");
    let filename = "invalid_filename.arrows";
    let file_path = create_test_file(&temp_dir, filename);
    let random_string = "random123";

    let result = arrow_path_to_parquet(&file_path, random_string);
    assert!(result.is_none());
}

#[test]
fn test_invalid_arrow_path_without_second_delimiter() {
    let temp_dir = TempDir::new().expect("Failed to create temp dir");
    let filename = "schema.nodelimiter.arrows";
    let file_path = create_test_file(&temp_dir, filename);
    let random_string = "random123";

    // This would fail with the current implementation due to the assert!
    // but should return None if the assert is replaced with a check
    let result = arrow_path_to_parquet(&file_path, random_string);
    assert!(result.is_none());
}


#[test]
fn get_or_create_returns_existing_stream() {
let streams = Streams::default();