gsmiller commented on PR #15823: URL: https://github.com/apache/lucene/pull/15823#issuecomment-4155783319
This looks mostly good to me, thanks! I left an earlier comment regarding consistency differences between this new addAll method and the existing addAll(Collection) method that I'd like to resolve (let's either, (a) decide the inconsistency is OK, (b) change the current addAll(Collection) behavior, or (c) open a follow-up issue to address it). The inconsistent behavior relates to how we handle an "overflow" case where more elements are provided than PQ capacity: - Current addAll(Collection) method: Detects the issue up-front and will not add any elements to the PQ. - New addAll(Stream) method: Because it cannot detect the issue up-front, it partially completes the operation by adding elements until capacity. I'd propose: Modify the current addAll(Collection) method to be consistent with the newly added addAll(Stream) method (this is the only option for consistency). (I think it's OK to do this in a follow-up issue if we want to separate the changes). I recognize this is fairly nitpicky, but I think it's helpful to maintain as much consistency as we can here. WDYT? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
