wgtmac commented on code in PR #3597:
URL: https://github.com/apache/parquet-java/pull/3597#discussion_r3497150029
##########
parquet-column/src/main/java/org/apache/parquet/internal/filter2/columnindex/RowRanges.java:
##########
@@ -316,4 +316,72 @@ public List<Range> getRanges() {
public String toString() {
return ranges.toString();
}
+
+ /**
+ * @return a new {@link Builder} for constructing a {@link RowRanges} from a
sequence of
+ * selected row indices.
+ */
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ /**
+ * Constructs a {@link RowRanges} by appending selected row indices in
strictly increasing
+ * order. Consecutive indices are coalesced into a single {@link Range};
gaps close the
+ * current run and start a new one.
+ *
+ * <p>Usage:
+ * <pre>{@code
+ * RowRanges.Builder builder = RowRanges.builder();
+ * for (long row : selectedRowsInOrder) {
+ * builder.addSelected(row);
+ * }
+ * RowRanges ranges = builder.build();
+ * }</pre>
+ */
+ public static class Builder {
+ private final List<Range> ranges = new ArrayList<>();
+ private long runStart = -1; // -1 = no active run
+ private long runEnd = -1; // valid iff runStart >= 0
+
+ /**
+ * Marks {@code blockRow} as selected. Must be called in strictly
increasing order; calling
+ * with a value less than or equal to the previous call's value throws
+ * {@link IllegalArgumentException}.
+ *
+ * @param blockRow the row index to mark selected (must be {@code >} the
last value passed)
+ * @return this builder for chaining
+ */
+ public Builder addSelected(long blockRow) {
Review Comment:
`addSelected` should reject negative row indexes explicitly. `-1` also
collides with the builder’s sentinel state, so invalid input can be silently
swallowed or corrupt the active run.
##########
parquet-column/src/main/java/org/apache/parquet/internal/filter2/columnindex/RowRanges.java:
##########
@@ -316,4 +316,72 @@ public List<Range> getRanges() {
public String toString() {
return ranges.toString();
}
+
+ /**
Review Comment:
This needs a clearer API commitment. The motivation is external
readers/Spark, but the type lives under `internal` and the PR says there are no
user-facing changes. If this is meant to be supported externally, the
package/API contract should say so; otherwise downstream users may depend on an
unsupported API.
##########
parquet-column/src/main/java/org/apache/parquet/internal/filter2/columnindex/RowRanges.java:
##########
@@ -316,4 +316,72 @@ public List<Range> getRanges() {
public String toString() {
return ranges.toString();
}
+
+ /**
+ * @return a new {@link Builder} for constructing a {@link RowRanges} from a
sequence of
+ * selected row indices.
+ */
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ /**
+ * Constructs a {@link RowRanges} by appending selected row indices in
strictly increasing
+ * order. Consecutive indices are coalesced into a single {@link Range};
gaps close the
+ * current run and start a new one.
+ *
+ * <p>Usage:
+ * <pre>{@code
+ * RowRanges.Builder builder = RowRanges.builder();
+ * for (long row : selectedRowsInOrder) {
+ * builder.addSelected(row);
+ * }
+ * RowRanges ranges = builder.build();
+ * }</pre>
+ */
+ public static class Builder {
Review Comment:
Consider making `Builder` `final` and adding an explicit constructor.
Otherwise subclassing and `new RowRanges.Builder()` become part of the public
API surface by accident.
##########
parquet-column/src/main/java/org/apache/parquet/internal/filter2/columnindex/RowRanges.java:
##########
@@ -316,4 +316,72 @@ public List<Range> getRanges() {
public String toString() {
return ranges.toString();
}
+
+ /**
+ * @return a new {@link Builder} for constructing a {@link RowRanges} from a
sequence of
+ * selected row indices.
+ */
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ /**
+ * Constructs a {@link RowRanges} by appending selected row indices in
strictly increasing
+ * order. Consecutive indices are coalesced into a single {@link Range};
gaps close the
+ * current run and start a new one.
+ *
+ * <p>Usage:
+ * <pre>{@code
+ * RowRanges.Builder builder = RowRanges.builder();
+ * for (long row : selectedRowsInOrder) {
+ * builder.addSelected(row);
+ * }
+ * RowRanges ranges = builder.build();
+ * }</pre>
+ */
+ public static class Builder {
+ private final List<Range> ranges = new ArrayList<>();
+ private long runStart = -1; // -1 = no active run
+ private long runEnd = -1; // valid iff runStart >= 0
+
+ /**
+ * Marks {@code blockRow} as selected. Must be called in strictly
increasing order; calling
+ * with a value less than or equal to the previous call's value throws
+ * {@link IllegalArgumentException}.
+ *
+ * @param blockRow the row index to mark selected (must be {@code >} the
last value passed)
+ * @return this builder for chaining
+ */
+ public Builder addSelected(long blockRow) {
Review Comment:
The naming could be clearer before this API ships. `addSelected(long
blockRow)` is ambiguous; something like `addSelectedRow(long rowIndex)` would
better communicate that the value is a 0-based row index within the current row
group.
##########
parquet-column/src/main/java/org/apache/parquet/internal/filter2/columnindex/RowRanges.java:
##########
@@ -316,4 +316,72 @@ public List<Range> getRanges() {
public String toString() {
return ranges.toString();
}
+
+ /**
+ * @return a new {@link Builder} for constructing a {@link RowRanges} from a
sequence of
+ * selected row indices.
+ */
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ /**
+ * Constructs a {@link RowRanges} by appending selected row indices in
strictly increasing
+ * order. Consecutive indices are coalesced into a single {@link Range};
gaps close the
+ * current run and start a new one.
+ *
+ * <p>Usage:
+ * <pre>{@code
+ * RowRanges.Builder builder = RowRanges.builder();
+ * for (long row : selectedRowsInOrder) {
+ * builder.addSelected(row);
+ * }
+ * RowRanges ranges = builder.build();
+ * }</pre>
+ */
+ public static class Builder {
+ private final List<Range> ranges = new ArrayList<>();
+ private long runStart = -1; // -1 = no active run
+ private long runEnd = -1; // valid iff runStart >= 0
+
+ /**
+ * Marks {@code blockRow} as selected. Must be called in strictly
increasing order; calling
+ * with a value less than or equal to the previous call's value throws
+ * {@link IllegalArgumentException}.
+ *
+ * @param blockRow the row index to mark selected (must be {@code >} the
last value passed)
+ * @return this builder for chaining
+ */
+ public Builder addSelected(long blockRow) {
+ if (runStart < 0) {
+ runStart = blockRow;
+ runEnd = blockRow;
+ } else if (blockRow == runEnd + 1) {
+ runEnd = blockRow;
+ } else if (blockRow > runEnd + 1) {
+ ranges.add(new Range(runStart, runEnd));
+ runStart = blockRow;
+ runEnd = blockRow;
+ } else {
+ throw new IllegalArgumentException(
+ "addSelected requires strictly increasing row indices; got " +
blockRow + " after " + runEnd);
+ }
+ return this;
+ }
+
+ /**
+ * @return the constructed {@link RowRanges}, or {@link RowRanges#EMPTY}
when no rows were
+ * selected.
+ */
+ public RowRanges build() {
Review Comment:
`build()` should return a snapshot or become terminal. As written, the
returned `RowRanges` shares the builder’s mutable `ranges` list, so later calls
on the same builder can mutate a previously built result and even break the
sorted-ranges invariant.
--
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]
