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

RepeatedField: unset by index #11425

Closed
wants to merge 6 commits into from

Conversation

Leprechaunz
Copy link
Contributor

Instead of using array_pop() to remove last element use unset() to remove by index

@Leprechaunz Leprechaunz requested a review from a team as a code owner December 29, 2022 11:55
@Leprechaunz Leprechaunz requested review from ericsalo and removed request for a team December 29, 2022 11:55
@google-cla
Copy link

google-cla bot commented Dec 29, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@Leprechaunz Leprechaunz marked this pull request as draft December 29, 2022 12:03
@Leprechaunz Leprechaunz marked this pull request as ready for review December 29, 2022 12:39
@haberman
Copy link
Member

Looks like the change is failing in CI.

@ericsalo ericsalo removed their request for review December 30, 2022 18:50
@Leprechaunz
Copy link
Contributor Author

@haberman I think it fixed it. Could you please run CI tests again?

@haberman
Copy link
Member

haberman commented Jan 4, 2023

Looks like it is still failing:

==5074== Memcheck, a memory error detector
==5074== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==5074== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==5074== Command: php -dextension=ext/google/protobuf/modules/protobuf.so vendor/bin/phpunit --bootstrap tests/force_c_ext.php tests
==5074==
PHPUnit 8.5.26 #StandWithUkraine

....WW....E.S..................................................  63 / 399 ( 15%)
............................................................... 126 / 399 ( 31%)
.............................WW................................ 189 / 399 ( 47%)
.....SS...WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW..WW.WWWW.....WW..... 252 / 399 ( 63%)
..SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS............... 315 / 399 ( 78%)
............................................................... 378 / 399 ( 94%)
.....................                                           399 / 399 (100%)

Time: 10.54 seconds, Memory: 0 bytes

There was 1 error:

1) ArrayTest::testInsertRemoval
Element at 3 doesn't exist.

@Leprechaunz
Copy link
Contributor Author

@haberman Thanks for logs. Should be fixed now.

Replaced unset with array_splice to reindex keys. But I'm sure this behaviour is correct for PHP.

If initial array was [0 => "a", 1 => "b", 2 => "c"] and we unset index 1 then we will result in [0 => "a", 1 => "c"]. But normal PHP unset should result in [0 => "a", 2 => "c"].

WDYT?

@bshaffer
Copy link
Contributor

bshaffer commented Jan 6, 2023

@Leprechaunz we should remain consistent with how PHP treats unset, and not reindex the array. Why not just use unset instead of array_splice?

@Leprechaunz
Copy link
Contributor Author

@bshaffer It's easy to do it for PHP. But extension written in C works differently.

RepeatedField uses upb_Array structure (basically array) and converts it into map later. We can set NULL, but not remove specific key.
I'm not a huge C expert, but only solution I see is to replace upb_Array with upb_Map in RepeatedField in extension implementation. Sounds like a huge change for small bug like this.

Maybe someone more experienced has better ideas?

@mcy mcy added the ruby label Jan 9, 2023
@bshaffer
Copy link
Contributor

bshaffer commented Jan 10, 2023

@Leprechaunz thank you for the explanation, that makes sense! It's also probably why the original implementation was written like it is (only unsetting the last element).

Since this is new behavior, I think the way you've implemented it makes sense (given that it isn't a breaking change and is consistent between the extension and the native implementation).

I'm not the maintainer of this library though, so it'll be up to them. Thanks for your work on this!

@Leprechaunz
Copy link
Contributor Author

@haberman Could you please re-run tests?

@haberman
Copy link
Member

The approach in this PR makes sense to me. Protobuf repeated fields are always dense: there is no way to have gaps or missing indices in the middle. So we definitely don't want a repeated field to use upb_Map.

@Leprechaunz
Copy link
Contributor Author

@haberman Sorry to bother you, but I updated my branch and re-run tests. Everything works localy. Could you please re-run tests?

@Leprechaunz
Copy link
Contributor Author

Merge of this PR is blocked because of "Tests / Check for Safety". Is there something I can do about it? Or maintainers will handle it?

@fowles fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 10, 2023
@fowles
Copy link
Contributor

fowles commented Feb 10, 2023

I think you need to rebase this onto our latest head. We have had a bunch of updates to our CI recently.

@mkruskal-google mkruskal-google added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Feb 10, 2023
alielbashir pushed a commit to alielbashir/protobuf that referenced this pull request Feb 14, 2023
Instead of using `array_pop()` to remove last element use `unset()` to remove by index

Closes protocolbuffers#11425

COPYBARA_INTEGRATE_REVIEW=protocolbuffers#11425 from Leprechaunz:unset-by-index a47fba5
PiperOrigin-RevId: 508691545
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants