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

throw if no fields #235

Merged
merged 2 commits into from
Sep 5, 2022
Merged

throw if no fields #235

merged 2 commits into from
Sep 5, 2022

Conversation

aplavin
Copy link
Member

@aplavin aplavin commented Jun 20, 2022

Before, StructArray([nothing]) or StructArray([1, 2, 3]) created empty (zero-length) arrays. That was silent and pretty confusing.

@aplavin
Copy link
Member Author

aplavin commented Aug 15, 2022

bump

src/utils.jl Outdated Show resolved Hide resolved
@piever
Copy link
Collaborator

piever commented Aug 15, 2022

Should this be considered breaking? In that case, may it would be best to coordinate merging this one with other breaking changes.

@aplavin
Copy link
Member Author

aplavin commented Aug 19, 2022

As I understand, the StructArray construction for types without fields just silently returned a wrong and totally unexpected result. This PR makes it an error.
Do you think the previous behavior was somehow useful in certain cases?

The ideal would be to actually support such types, but I don't see a simple place to plug that support in.

@piever
Copy link
Collaborator

piever commented Aug 22, 2022

Do you think the previous behavior was somehow useful in certain cases?

I think IndexedTables used the "StructArray with no fields" in some cases (cc: @shashi), which is why the constructor is there in the first place, but I confess I don't remember the exact usecase.

@aplavin
Copy link
Member Author

aplavin commented Aug 26, 2022

Took all comments into account in the recent commit.

Also see #242, alternative to this PR. Here I make it throw an error instead of a silently wrong behavior, and #242 makes no-field eltypes actually work.

@piever
Copy link
Collaborator

piever commented Sep 2, 2022

Thanks for finalizing this. I think I'd prefer this over #242. It might make sense to combine this and #243 (which also seems very slightly breaking) into the next breaking release.

@aplavin
Copy link
Member Author

aplavin commented Sep 2, 2022

Great, looking forward for this to be released!
Is this PR really breaking? It looks strictly as a bugfix to me: unsupported arguments throw an error instead of returning an obviously wrong result.
As for #243, technically it could break someones code with very strict component type checks, but these things are regularly considered non-breaking even by base julia.

There were multiple changes recently that are arguably more breaking, but didn't get a breaking release.
Take #239 for example: if someone uses StaticArrays <= 1.5.3, they'll have StructArrays not working with FieldArrays properly.
Or #227 breaking some fancy array types, as discussed in #228.
Almost any change is technically breaking for some.
Maybe, a good indicator for usefully breaking/non-breaking release is whether any existing tests were modified?

Anyway, a breaking release would also be fine for me.

@piever piever merged commit 01dd3e3 into JuliaArrays:master Sep 5, 2022
piever pushed a commit that referenced this pull request Sep 5, 2022
* throw if no fields

* after code review
@aplavin aplavin deleted the patch-2 branch September 13, 2022 11:13
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.

2 participants