Skip to content

fix: ak.Record creation from iterable with nested dicts#3728

Merged
pfackeldey merged 5 commits into
scikit-hep:mainfrom
ikrommyd:ikrommyd/fix-record-creation
Dec 10, 2025
Merged

fix: ak.Record creation from iterable with nested dicts#3728
pfackeldey merged 5 commits into
scikit-hep:mainfrom
ikrommyd:ikrommyd/fix-record-creation

Conversation

@ikrommyd

Copy link
Copy Markdown
Member

Closes #3723

I think is_non_string_like_iterable is not enough here. It should also not be a dictionary cause dictionary is iterable.
However, dicts I think should be wrapped into a single-element list like line 1864 does right below.

@codecov

codecov Bot commented Nov 13, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.71%. Comparing base (b749e49) to head (052af3a).
⚠️ Report is 486 commits behind head on main.

Additional details and impacted files
Files with missing lines Coverage Δ
src/awkward/highlevel.py 78.30% <100.00%> (+1.63%) ⬆️

... and 199 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown

The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3728

@ikrommyd ikrommyd changed the title fix: ak.Record creation with nested dicts fix: ak.Record creation from iterable with nested dicts Nov 13, 2025

@ianna ianna left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ikrommyd - thanks for jumping in. Let’s discuss this at the next meeting. Thanks

@pfackeldey pfackeldey left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @ikrommyd, this is the correct fix. This additional check avoids calling the ak.Array constructor on a scalar (there we prohibit scalar promotion in to_layout). The else branch on the other hand wraps v into a list so that ak.to_layout properly operates on an iterable instead of a scalar. Also, this fix works for any nested level because the ak.Array constructor is called recursively.

This looks good to me, I only like to ask you to add the original issue as a test. Maybe it's also good to add a comment above the if condition saying "avoid dictionaries here, see issue #3723" as it may not seem obvious why this additional check is needed at first sight.

After that, this is ready to be merged 👍

@ikrommyd

ikrommyd commented Dec 9, 2025

Copy link
Copy Markdown
Member Author

Both done @pfackeldey! I did not fully understand why we loop manually over the dict and not use from_iter in this code path though. Maybe going into the array builder is slower? Idk. Anyways, it should be ready.

@ikrommyd
ikrommyd requested a review from pfackeldey December 9, 2025 15:14

@pfackeldey pfackeldey left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Great!

@pfackeldey
pfackeldey merged commit 87765b6 into scikit-hep:main Dec 10, 2025
42 checks passed
@ikrommyd
ikrommyd deleted the ikrommyd/fix-record-creation branch December 10, 2025 20:01
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.

ak.Record rejects nested dicts with scalar leaves

3 participants