Copilot commented on code in PR #2530:
URL: https://github.com/apache/phoenix/pull/2530#discussion_r3409104001
##########
phoenix-core-client/src/main/java/org/apache/phoenix/parse/ExplainOptions.java:
##########
@@ -79,49 +79,4 @@ public String toString() {
return "ExplainOptions{regions=" + regions + ", verbose=" + verbose + ",
format=" + format
+ "}";
}
-
- /**
- * Mutable builder used by the parser to accumulate options as they are
encountered in the option
- * list. Rejects duplicate options.
- */
- public static final class Builder {
- private boolean regions;
- private boolean regionsSet;
- private boolean verbose;
- private boolean verboseSet;
- private Format format;
-
- public Builder setRegions(boolean regions) {
- if (regionsSet) {
- throw new RuntimeException("Duplicate EXPLAIN option: REGIONS");
- }
- this.regions = regions;
- this.regionsSet = true;
- return this;
- }
-
- public Builder setVerbose(boolean verbose) {
- if (verboseSet) {
- throw new RuntimeException("Duplicate EXPLAIN option: VERBOSE");
- }
- this.verbose = verbose;
- this.verboseSet = true;
- return this;
- }
-
- public Builder setFormat(Format format) {
- if (format == null) {
- throw new RuntimeException("EXPLAIN option FORMAT requires a value:
TEXT or JSON");
- }
- if (this.format != null) {
- throw new RuntimeException("Duplicate EXPLAIN option: FORMAT");
- }
- this.format = format;
- return this;
- }
-
- public ExplainOptions build() {
- return new ExplainOptions(regions, verbose, format == null ? Format.TEXT
: format);
- }
- }
}
Review Comment:
Removing the public nested `ExplainOptions.Builder` is a
source/binary-incompatible API change for any downstream code that may have
been constructing `ExplainOptions` via the builder. If Phoenix client APIs aim
to maintain compatibility across releases, consider keeping `Builder` as a
deprecated wrapper that delegates to the `ExplainOptions` constructor and
removing it in a major version instead.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/iterate/ExplainTable.java:
##########
@@ -804,14 +803,13 @@ private String getRegionLocationsForExplainPlan(
}
try {
StringBuilder buf = new StringBuilder().append(REGION_LOCATIONS);
- Set<RegionBoundary> regionBoundaries = new HashSet<>();
+ Set<String> regionBoundaries = new LinkedHashSet<>();
List<HRegionLocation> regionLocations = new ArrayList<>();
for (HRegionLocation regionLocation : regionLocationsFromResultIterator)
{
- RegionBoundary regionBoundary = new
RegionBoundary(regionLocation.getRegion().getStartKey(),
- regionLocation.getRegion().getEndKey());
- if (!regionBoundaries.contains(regionBoundary)) {
+ String regionBoundary =
Bytes.toStringBinary(regionLocation.getRegion().getStartKey()) + ":"
+ + Bytes.toStringBinary(regionLocation.getRegion().getEndKey());
Review Comment:
`regionBoundary` is built by concatenating two `Bytes.toStringBinary(...)`
values with a ":" delimiter. Since `toStringBinary` can itself contain colons
for printable bytes, different (startKey,endKey) pairs can collide to the same
string (e.g. start="a:", end="b" vs start="a", end=":b"), causing incorrect
de-duplication and potentially missing region locations in EXPLAIN output. Use
a delimiter that cannot appear in `toStringBinary` output (e.g. NUL) or encode
with length prefixes.
--
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]
