Skip to content

Conversation

@aokolnychyi
Copy link
Contributor

Until we store the output type of transforms, we should fail building the partition type if we hit UnknownTransform.

@aokolnychyi
Copy link
Contributor Author

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

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

This looks good to me overall. A few questions for my own understanding.

A few questions:

  1. I'm assuming you're using this subset of metadata tables because they're partitioned?
  2. Is there a way to add a test against a table itself (and not just the metadata tables)?

@aokolnychyi
Copy link
Contributor Author

@kbendick, this is a follow-up to #2936 where I added Partitioning#partitionType. It is only used within these metadata tables that I added tests for. W.r.t. reading the table itself, we have TestForwardCompatibility that already covers writing and reading to a table with unknown transforms.

Copy link
Contributor

@yyanyy yyanyy left a comment

Choose a reason for hiding this comment

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

I guess I'm not familiar with the context, is this case only going to be hit when there's a new writer that commit partition spec to the table that contains a new transform, and the metadata tables associated with the table is read by the old reader which does not recognize this transform; and we are updating the logic for this old reader to fail gracefully?

@aokolnychyi
Copy link
Contributor Author

@yyanyy, that's exactly the use case.

Copy link
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

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

this also looks good to me

@aokolnychyi aokolnychyi merged commit 56a2f3b into apache:master Aug 18, 2021
@aokolnychyi
Copy link
Contributor Author

Thanks for reviewing, @kbendick @yyanyy @jackye1995!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants