Skip to content

fillData sorted data assumption fixed #3062

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

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

Conversation

Dharmesh-Kota
Copy link

@Dharmesh-Kota Dharmesh-Kota commented Jul 9, 2025

Description

This PR fixes a critical bug in the fillData helper in victory-stack, which incorrectly assumes that datasets passed into it are already sorted by _x. In many real-world cases, including the one demonstrated in Issue #3061, data may be unsorted. The previous implementation relied on array index alignment (index - indexOffset), which fails when data is unordered, leading to incorrect mapping of data points and insertion of unnecessary synthetic values.

Key Improvements:

  • Introduced a Map-based lookup (dataMap) for _x values instead of index tracking.
  • Ensured that missing values are correctly inserted when not present, even if input datasets are unsorted.
  • Removed reliance on the assumption that data is sorted ahead of time.
  • The updated implementation uses a Map for constant-time (O(1)) lookup of original data values by _x and avoids unnecessary scanning or mismatched indexing logic

Results in more predictable and performant iteration, especially with larger or unsorted datasets

Fixes # (issue)

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Locally tested with real-world datasets containing unsorted _x values.
  • Validated by comparing results of fillData before and after the patch using sample datasets.
  • Ensured correct reconstruction of series with missing x-values using patched logic.

Copy link

changeset-bot bot commented Jul 9, 2025

⚠️ No Changeset found

Latest commit: 8a2ca37

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jul 9, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
victory ✅ Ready (Inspect) Visit Preview Jul 9, 2025 2:34am

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