Modify partition_values field type in Add/Remove actions.

[zijie0]

Description

Modify partition_values to type HashMap<String, Option> according to the delta spec.

Related Issue(s)

• closes Read table failed when containing null partition values #348

Documentation

Detailed discussion here: https://groups.google.com/g/delta-users/c/ZzzrtNRqRyk

[houqp]

Thank you @zijie0 ! Looks good from a quick glance, I will take a closer look later today.

[zijie0]

@houqp I'm not sure whether I need to add some write tests. I tried locally changed following test: https://github.com/delta-io/delta-rs/blob/main/rust/tests/write_exploration.rs#L422 with empty string value:

// test data set #2 - updates
let json_rows = vec![
    json!({ "id": "D", "value": 148, "modified": "" }),
    json!({ "id": "E", "value": 150, "modified": "" }),
    json!({ "id": "F", "value": 152, "modified": "" }),
];

The generated files seems to be the same with "native" delta.

[houqp]

This reminded me that we are doing partition filtering wrong in get_files_by_partitions. We are parsing Action.path to reconstruct the partition values, but instead, we should be using Action.partition_values or Action.partition_values_parsed directly.

I will file a separate issue to track this.

[houqp]

cc @fvaleye #358.

[houqp]

It occurred to me that the problem you found has a much bigger impact than just the partition_values field, it also applies to Add.tags, Remove.tags, MetaData.format.options, MetaData.configurations.

Let me know if you prefer to address them or only focus on partition_values in this PR. If you prefer the latter, I can file an issue to track the fix for the other fields.

[zijie0]

Hey QP, I went through the spec doc but didn't found situations that those structs could also contain null values in the map. Is that because Map[String, String] allows null value in Scala, so that we also need to make the change to HashMap<String, Option<String>>?

[houqp]

That's correct, based on the discussion in the dev list, all data types are annotated using scala syntax, so all Map[String, String] should map to HashMap<String, Option<String>> in rust.

[houqp]

btw, I am perfectly fine with us addressing these other fields in an other PR if you want to keep the scope of this PR small. The fix for partition_values itself is already a big improvement :)

[zijie0]

Let me take a look on this :)

[houqp]

@houqp I'm not sure whether I need to add some write tests.

@zijie0 it would be great if you could help write some write tests to cover the cases of both empty strings and actual null values. So perhaps something like below:

    json!({ "id": "D", "value": 148, "modified": "" }),
    json!({ "id": "E", "value": 150, "modified": null }),

[houqp]

one minor ask, could we check the partition_values field instead to make sure it's null? delta doesn't care whether we have partition value in the file path or not, so it's perfectly valid to not have modified=__HIVE_DEFAULT_PARTITION_ in the path.

[zijie0]

Good point! I will change the assertion.

[houqp]

The rest looks good to me, thank you @zijie0 for discovering this big can of worms :)

[zijie0]

It is weird, cargo clippy passed on my local machine. Could you help me on this build failure? @houqp

[zijie0]

Sorry, forgot to rebase on main branch...

[houqp]

Thank you @zijie0 !