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

[rfc] Fixing MultiPartitionsDefinition subset -> range -> keys inconsistency #26652

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jamiedemaria
Copy link
Contributor

@jamiedemaria jamiedemaria commented Dec 20, 2024

Summary & Motivation

This bug reported a backfill of a multi-partitioned asset never completing #26307. I was able to replicate this issue and believe the cause is that we are losing information when we convert a sequence of MultiParitition keys to a subset, then to a range, then back to a list of keys. I've written up a test case demonstrating the behavior.

It looks like this conversion used to work until #19448 which was meant to improve the performance of the fn (so it doesn't have to iterate through all partition keys)

I'm not sure what the right way to resolve this is. We could go back to the old version for converting a range to a list of keys, but that loses the performance gains of #19448 .


(Less important to this PR, but documenting how this manifests as an issue for backfills so that we don't lose context over the holidays)

In backfills this manifests like this:

  • We have a list of MultiPartition keys to materialize for an asset: 2023-01-01|a, 2023-01-01|b, 2023-01-01|c, 2023-01-02|a
  • We convert those keys into a subset, and then generate the ranges to determine which partitions can be materialized together in a single run. The get_partition_key_ranges returns the range [PartitionKeyRange(start='2023-01-01|a', end='2023-01-02|a')]. This is accurate since if you list the partition keys in order, the sequence goes 2023-01-01|a, 2023-01-01|b, 2023-01-01|c, 2023-01-02|a, 2023-01-02|b, ...
  • We launch a single run for the range [PartitionKeyRange(start='2023-01-01|a', end='2023-01-02|a')]`
  • On the next tick of the backfill daemon, we want to see which partitions were materialized. When we get the materialization events for the asset, we only see events for partitions 2023-01-01|a and 2023-01-02|a. I believe this is because we are calling get_partition_keys_in_range when determining what materialization events to emit for the ranged run (still tracking down where this happens in the code)
  • There's an additional layer of logic in the backfill daemon that won't see that the runs for 2023-01-01|b, 2023-01-01|c because they are root assets for the backfill, and we could make some changes to see that they didn't get materialized. But the core bug is with these conversion methods

How I Tested These Changes

Changelog

Insert changelog entry or delete this section.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant