fix: use physical name for column name lookup in partitions

[aersam]

Description

get_actions wrongly assumes that partition_columns from schema and partitionValues from log must be the same. This is not true since partition_columns are logical column names while partitionValues are physical column names.

Tests pending

Related Issue(s)

• closes get_add_actions fails with deltalake 0.13 #1835

Documentation

https://github.com/delta-io/delta/blob/master/PROTOCOL.md#writer-requirements-for-column-mapping
"Track partition values and column level statistics with the physical name of the column in the transaction log."

[github-actions]

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

[aersam]

seems to be compiling in CI/CD.

Where would you propose to add a test for this?

Would also have to check if the issue exists with statistics as well

[wjones127]

For testing, I think the tests belong here:

delta-rs/crates/deltalake-core/src/protocol/mod.rs

Line 858 in 9b93830

mod arrow_tests {

[aersam]

For testing, I think the tests belong here:

delta-rs/crates/deltalake-core/src/protocol/mod.rs

Line 858 in 9b93830

mod arrow_tests {

Ok, I'll add some tests. How am I supposed to execute those locally? Always get strange compile errors locally. I'm using Windows

[roeap]

Always get strange compile errors locally. I'm using Windows

There is currently a build issue on main which may be related. Could you try with the datafusion flag enabled?

[aersam]

Always get strange compile errors locally. I'm using Windows

There is currently a build issue on main which may be related. Could you try with the datafusion flag enabled?

Does not help, however in WSL it works just with cargo test. Tests seem to be passing now, so if you want to review ;)

[aersam]

I don't think the build error is related to my changes? At least I would not understand it 🙂

[roeap]

Thanks @aersam for getting us started on column mapping!

Left a comment around error handling, but overall looking good.

One thing I noticed is that there is a lot going on in the example data log. Would it maybe be an option to set up the column mapping on creating the table, which makes it a bit easier to reason about the relevant commit.

[aersam]

Thanks @aersam for getting us started on column mapping!

Left a comment around error handling, but overall looking good.

One thing I noticed is that there is a lot going on in the example data log. Would it maybe be an option to set up the column mapping on creating the table, which makes it a bit easier to reason about the relevant commit.

I now simplified the test file, it's a lot less files now.
So it's ready for the next review :)

[aersam]

Always get strange compile errors locally. I'm using Windows

There is currently a build issue on main which may be related. Could you try with the datafusion flag enabled?

One comment here: the error disappeared on Windows when doing cargo update

[roeap]

Thanks for the log clean-up - much easier to read now 👍.

Clarified a bit on why to remove the panic and handle via a Result.

[aersam]

Thanks for the log clean-up - much easier to read now 👍.

Clarified a bit on why to remove the panic and handle via a Result.

In addition to a lot more error handling, I also added a check for delta.columnMapping.mode which matches the spec. If column mapping mode=none, we should ignore physical name entirely

[roeap]

Looking really good!

One more ask is to handle the column mapping analogous to the rest of the table configuration.

[roeap]

Thanks for sticking with it and your patience @aersam!

[aersam]

Thanks for sticking with it and your patience @aersam!

Thanks for your guidance and for merging it!