Copilot commented on code in PR #477:
URL: https://github.com/apache/hudi-rs/pull/477#discussion_r2471613177
##########
crates/core/src/file_group/builder.rs:
##########
@@ -158,9 +161,10 @@ mod tests {
.clone();
let result = build_file_groups(&metadata);
- assert!(matches!(result,
- Err(CoreError::CommitMetadata(msg))
- if msg == "Invalid or missing partitionToWriteStats object"));
+ // With the new implementation, this returns Ok with an empty
HashSet
+ // because iter_write_stats() returns an empty iterator when
partition_to_write_stats is None
Review Comment:
[nitpick] The comment incorrectly states the behavior is new. This appears
to be documenting changed behavior in an existing test. Consider updating the
comment to explain why this behavior is correct rather than just describing the
implementation detail.
```suggestion
// When partition_to_write_stats is missing from the metadata,
// the correct behavior is to return Ok with an empty HashSet,
since there are no file groups to build.
```
##########
crates/core/src/file_group/builder.rs:
##########
@@ -367,9 +373,10 @@ mod tests {
.clone();
let result = build_replaced_file_groups(&metadata);
+ // Serde will fail to parse this
Review Comment:
[nitpick] The comment is incomplete and doesn't explain the test's purpose.
Consider explaining what invalid data scenario this test is validating.
```suggestion
// This test verifies that if the file IDs array contains a
non-string value (here, a number instead of a string),
// Serde will fail to parse the commit metadata and return the
appropriate error.
```
##########
crates/core/src/file_group/builder.rs:
##########
@@ -257,9 +261,10 @@ mod tests {
.clone();
let result = build_file_groups(&metadata);
+ // Serde will fail to parse this and return a deserialization error
Review Comment:
[nitpick] The comment explains implementation details but doesn't clarify
why this test case is important. Consider explaining what invalid data scenario
this test is validating.
```suggestion
// This test validates that providing a non-string value for the
'fileId' field (here, a number) results in a deserialization error, as 'fileId'
is expected to be a string.
```
##########
crates/core/src/file_group/builder.rs:
##########
@@ -385,9 +392,10 @@ mod tests {
.clone();
let result = build_replaced_file_groups(&metadata);
+ // Serde will fail to parse this
Review Comment:
[nitpick] The comment is incomplete and doesn't explain the test's purpose.
Consider explaining what invalid data scenario this test is validating.
```suggestion
// This tests the scenario where the array of file IDs contains
a null value,
// which is invalid. Serde should fail to parse this as a valid
file ID.
```
##########
crates/core/src/file_group/builder.rs:
##########
@@ -278,9 +283,10 @@ mod tests {
.clone();
let result = build_file_groups(&metadata);
+ // Serde will fail to parse this and return a deserialization error
Review Comment:
[nitpick] The comment explains implementation details but doesn't clarify
why this test case is important. Consider explaining what invalid data scenario
this test is validating.
```suggestion
// This test verifies that deserialization fails when the "path"
field is not a string,
// ensuring that invalid data types in commit metadata are
properly rejected.
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
