Skip to content

Add a replaceColumn method to Page#22493

Merged
elharo merged 1 commit intomasterfrom
replace
Apr 13, 2024
Merged

Add a replaceColumn method to Page#22493
elharo merged 1 commit intomasterfrom
replace

Conversation

@elharo
Copy link
Contributor

@elharo elharo commented Apr 11, 2024

Description

Replace a column (block) in a page with a new column/bliock.

Motivation and Context

#22078 For row IDs we need to replace a row number block with a row ID block. however the functionality seems generally useful and has no particular dependence on row IDs.

Impact

Add new replaceColumn method to com.facebook.common.Page

Test Plan

Added unit tests to TestPage class

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== RELEASE NOTES ==

SPI
* com.facebook.common.Page has a new replaceColumn method.

@elharo elharo marked this pull request as ready for review April 11, 2024 19:53
@elharo elharo requested a review from a team as a code owner April 11, 2024 19:53
@elharo elharo force-pushed the replace branch 2 times, most recently from eb2d8aa to a4f13de Compare April 11, 2024 20:23
@steveburnett
Copy link
Contributor

Nit: suggest revising the release note entry to follow the Order of changes in the release note guidelines.

Maybe:

== RELEASE NOTES ==

General Changes
* Add new replaceColumn method to com.facebook.common.Page

public Page replaceColumn(int channelIndex, Block column)
{
if (column.getPositionCount() != positionCount) {
throw new IllegalArgumentException("New column does not have same number of rows as old column");
Copy link
Collaborator

@sdruzkin sdruzkin Apr 12, 2024

Choose a reason for hiding this comment

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

Replace: New column does not have same number of rows as old column -> New block does not have the same number of rows as the page

Technically current page implementation allows to have blocks with different number of rows, but I think it's ok to enforce this constraint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

appendColumn and prependColumn do check for this though interestingly, as you point out, the constructor does not.

* @return a new page with the replacement column substituted for the old column
*/
public Page replaceColumn(int channelIndex, Block column)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check and throw if the channelIndex is invalid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are unit tests that verify the correct exception is thrown in this case.

@elharo elharo merged commit 6c2bf82 into master Apr 13, 2024
@elharo elharo deleted the replace branch April 13, 2024 10:12
@rschlussel
Copy link
Contributor

For the release note, it should be in a section titled SPI.

@wanglinsong wanglinsong mentioned this pull request May 1, 2024
48 tasks
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.

5 participants