Skip to content

Conversation

@zhangshenghang
Copy link
Member

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@github-actions github-actions bot removed the api label Nov 2, 2025
@zhangshenghang zhangshenghang marked this pull request as ready for review November 2, 2025 13:11
Copy link
Contributor

@davidzollo davidzollo left a comment

Choose a reason for hiding this comment

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

+1

.withDescription(
"Whether to include the start row in the scan. Default is true (inclusive).");

public static final Option<Boolean> END_ROW_INCLUSIVE =
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to maintain consistency with the previous logic, i think the default value of end_row_inclusive should also be true.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @xiaochen-zhou suggest, to ensure that data can be read normally without duplication, we need to include only one side.

Copy link
Member

Choose a reason for hiding this comment

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

hi,Will the data be duplicated when both END_ROW_INCLUSIVE and START_ROW_INCLUSIVE are true?

Copy link
Member

Choose a reason for hiding this comment

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

I think the intended result of the configuration should be to control whether the boundaries are included. However, I believe that when both sides are set to false, it will result in shards like (start, 1), (1, 3), (3, end), which causes the loss of boundary data. Conversely, it will lead to duplicate boundary data. Instead of adjusting the boundaries of each split, we should only modify the inclusion settings of the start and end points

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants