Copilot commented on code in PR #2538:
URL: https://github.com/apache/phoenix/pull/2538#discussion_r3439724178
##########
phoenix-core-client/src/main/java/org/apache/phoenix/iterate/ExplainTable.java:
##########
@@ -772,14 +789,14 @@ private static String detectPathTestSubtag(Expression
expression) {
private void getRegionLocations(List<String> planSteps,
ExplainPlanAttributesBuilder explainPlanAttributesBuilder,
List<HRegionLocation> regionLocations) {
- // Region locations are emitted as text and in the structured attributes
only when the
- // EXPLAIN statement requested them via the REGIONS option (or the legacy
WITH REGIONS alias).
- if (!context.getExplainOptions().isRegions()) {
- return;
- }
+ // Region locations are computed during scan planning, so always record
them in the structured
+ // attributes. Consumers that read the attributes directly (e.g. the
connection activity logger,
+ // and JSON output) rely on them being present. Only append them to the
text plan steps when
+ // the EXPLAIN statement requested them via the REGIONS option (or the
legacy WITH REGIONS
+ // alias).
String regionLocationPlan =
getRegionLocationsForExplainPlan(explainPlanAttributesBuilder,
regionLocations);
- if (regionLocationPlan.length() > 0) {
+ if (context.getExplainOptions().isRegions() && regionLocationPlan.length()
> 0) {
planSteps.add(regionLocationPlan);
}
Review Comment:
`getRegionLocationsForExplainPlan(...)` always constructs the
region-locations plan string (including list `toString()` work) even when
`EXPLAIN ... REGIONS` is not requested. Since the intent is only to always
populate structured attributes, consider splitting “populate attributes” from
“render text” so we avoid building the (potentially large) text representation
unless it will be appended to `planSteps`.
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/GlobalIndexCheckerIT.java:
##########
@@ -293,10 +314,15 @@ public void testPhoenixRowTimestamp() throws Exception {
String noIndexQuery = "SELECT /*+ NO_INDEX */ val1, val2,
PHOENIX_ROW_TIMESTAMP() from "
+ dataTableName + " WHERE val1 = 'bc' AND " + "PHOENIX_ROW_TIMESTAMP()
> TO_DATE('"
+ after.toString() + "','yyyy-MM-dd HH:mm:ss.SSS', '" + timeZoneID +
"')";
- // Verify that we will read from the data table
+ // Verify that we will read from the data table. The NO_INDEX hint
rejects every secondary
+ // index candidate. Under STRONG consistency only the user index exists.
Under EVENTUAL
+ // consistency the user index is paired with an auto-created CDC index,
so two candidates are
+ // rejected. Match the user index by name rather than position since the
rejection order is
+ // not guaranteed.
Review Comment:
The new NO_INDEX hint explanation comment lines appear to exceed the
100-character Checkstyle LineLength limit once indentation is included. This
will likely fail checkstyle; please wrap the comment to shorter lines.
--
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]
