[BugFix] Fix bin/chart NPE with null values (#5174) (#5334)

* [BugFix] Fix bin/chart NPE with null values (#5174)

Bin bucket functions declared non-nullable VARCHAR return type, causing
Calcite to optimize away IS NOT NULL filters and allowing null group
keys to reach TreeMap which crashes in compareTo.

Fix: declare return type as nullable using FORCE_NULLABLE and add
nullsLast to chart command sorts as a defensive measure.

Signed-off-by: Heng Qian <qianheng@amazon.com>

* Remove stale /dedupe section from CLAUDE_GUIDE.md

The dedupe command and scripts/comment-on-duplicates.sh were removed
from main, but CLAUDE_GUIDE.md still referenced them, causing
linkchecker CI failure.

Signed-off-by: Heng Qian <qianheng@amazon.com>

* Fix incorrect YAML test assertion and add datarows validation

- Fix total count from 3 to 2 in YAML REST test (bin+chart with
  category produces 2 groups, not 3)
- Add datarows assertions to both YAML test cases for row-level
  validation
- Correct table statistics row count in unit test

Signed-off-by: Heng Qian <qianheng@amazon.com>

* Update ppl-bugfix-reference: prefer datarows match over length check

YAML REST tests should assert actual row content, not just count.
Learned from #5174 where total-only assertion missed incorrect values.

Signed-off-by: Heng Qian <qianheng@amazon.com>

* Improve harness: worktree tracking, completion gate, pwd reporting

- Add pwd/branch reporting at harness start so caller knows the
  working directory
- Add Completion Gate with git status check to prevent uncommitted
  changes from being left behind
- Skill Step 2B reuses existing worktree if PR branch is already
  checked out, instead of always creating a new one
- Step 3 requires main agent to record worktree→PR mapping for
  directing future operations to the correct directory

Signed-off-by: Heng Qian <qianheng@amazon.com>

* Add CalciteBinChartNullIT to CalciteNoPushdownIT suite

bin+chart with null values should also be tested with pushdown disabled.

Signed-off-by: Heng Qian <qianheng@amazon.com>

* Prefer adding tests to existing IT suites over creating new ones

Note in harness Phase 2 that new IT classes should be added to
CalciteNoPushdownIT suite.

Signed-off-by: Heng Qian <qianheng@amazon.com>

---------

Signed-off-by: Heng Qian <qianheng@amazon.com>

Author: qianheng-aws

.claude/commands/ppl-bugfix.md

@@ -83,6 +83,33 @@ Agent(
 
 ### 2B: Follow-up
 
+Before dispatching, check if an existing worktree already has the PR branch checked out:
+
+```bash
+# List worktrees and find one on the PR branch
+for wt in .claude/worktrees/agent-*/; do
+  branch=$(git -C "$wt" branch --show-current 2>/dev/null)
+  if [ "$branch" = "<pr_branch>" ]; then
+    echo "REUSE: $wt (branch: $branch)"
+  fi
+done
+```
+
+**If existing worktree found**: Do NOT use `isolation: "worktree"`. Pass the worktree path in the prompt so the subagent works there directly.
+
+```
+Agent(
+  mode: "<resolved_mode>",
+  name: "bugfix-<issue_number>",
+  description: "PPL bugfix #<issue_number> followup",
+  prompt: "cd <worktree_path> first, then read .claude/harness/ppl-bugfix-followup.md and follow it.
+           PR: <pr_number> (<pr_url>), Issue: #<issue_number>
+           Working directory: <worktree_path>"
+)
+```
+
+**If no existing worktree**: Create a new one.
+
 ```
 Agent(
   mode: "<resolved_mode>",
@@ -100,6 +127,16 @@ After all subagents complete, report a summary for each:
 - Classification, fix summary, PR URL, worktree path and branch, items needing human attention (2A)
 - What was addressed, current PR state, whether another round is needed (2B)
 
+**Always include the worktree→PR mapping** from the subagent's output, e.g.:
+
+```
+Worktree: /path/to/.claude/worktrees/agent-xxxx
+Branch: bugfix-1234
+PR: #5678
+```
+
+**Important**: After reporting, the main agent must remember this mapping. When the user later asks to make changes to the PR (e.g., "commit this to PR #5678"), operate in the worktree directory — not the main session directory.
+
 ## Subagent Lifecycle
 
 Subagents are task-scoped. They complete and release context — they cannot poll for events.

.claude/harness/ppl-bugfix-followup.md

@@ -6,9 +6,18 @@
 
 ---
 
+## Report Working Directory
+
+```bash
+echo "Worktree: $(pwd)"
+echo "Branch: $(git branch --show-current)"
+```
+
+Include this in your output so the caller knows where changes are happening.
+
 ## Reconstruct Context
 
-The follow-up agent runs in a fresh worktree. First checkout the PR branch, then load state:
+First checkout the PR branch, then load state:
 
 ```bash
 # Checkout the PR branch in this worktree
@@ -101,3 +110,16 @@ For each comment addressed (bot or human):
 - **Did the follow-up workflow itself miss this signal?** → Update this file
 
 If any improvement is needed, make the edit and include it in the same commit.
+
+## Completion Gate
+
+Before reporting "done":
+
+1. Run `git status --porcelain` — if any uncommitted changes remain, commit and push them. This includes harness edits from Retrospective.
+2. Report in your final output:
+
+```
+Worktree: <absolute path>
+Branch: <branch name>
+PR: <pr_number>
+```

.claude/harness/ppl-bugfix-harness.md

@@ -2,6 +2,15 @@
 
 ## Phase 0: Triage
 
+### 0.0 Report Working Directory
+
+```bash
+echo "Worktree: $(pwd)"
+echo "Branch: $(git branch --show-current)"
+```
+
+Include this in your output so the caller knows where changes are happening.
+
 ### 0.1 Load & Reproduce
 
 ```bash
@@ -53,7 +62,7 @@ Consult `.claude/harness/ppl-bugfix-reference.md` for test templates.
 Required deliverables:
 - Failing test reproducing the bug (written BEFORE the fix)
 - Unit tests covering happy path and edge cases
-- Integration test (`*IT.java` extending `CalcitePPLIT`)
+- Integration test — add to an existing `*IT.java` when possible; if creating a new one, add it to `CalciteNoPushdownIT`
 - YAML REST test at `integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/<ISSUE>.yml`
 
 ---
@@ -126,6 +135,8 @@ EOF
 
 ## Completion Gate
 
+Run `git status --porcelain` — if any uncommitted changes remain, commit and push them before proceeding.
+
 Do NOT report "done" until every item below is checked. List each in your final report:
 
 - [ ] **Unit tests**: New test class or methods
@@ -149,3 +160,10 @@ If any item is blocked, report which and why.
 - [ ] New pattern? Add to Case Index.
 
 Include harness improvements in the same PR.
+
+Report in your final output:
+```
+Worktree: <absolute path>
+Branch: <branch name>
+PR: <pr_number>
+```

.claude/harness/ppl-bugfix-reference.md

@@ -109,9 +109,13 @@ teardown:
       headers: { Content-Type: 'application/json' }
       ppl: { body: { query: "source=test_issue_<ISSUE> | <your PPL>" } }
   - match: { total: <expected> }
-  - length: { datarows: <expected> }
+  - match: { datarows: [ [ <row1_val1>, <row1_val2> ], [ <row2_val1>, <row2_val2> ] ] }
 ```
 
+> **Always include `datarows` assertions** — verifying only `total` and `schema` will miss
+> wrong values. Count the expected output groups carefully (e.g., for `chart ... by <col>`,
+> count distinct (row_split, col_split) groups after null filtering, not the number of input rows).
+
 ---
 
 ## Symptom → Fix Path

CLAUDE_GUIDE.md

@@ -30,25 +30,3 @@ Fix a PPL bug end-to-end or follow up on an existing PR.
 
 **Related files:** [`.claude/harness/ppl-bugfix-harness.md`](.claude/harness/ppl-bugfix-harness.md)
 
----
-
-## `/dedupe`
-
-Find duplicate GitHub issues for a given issue.
-
-**Usage:**
-
-```
-/dedupe 1234
-```
-
-**What it does:**
-
-1. Reads the target issue
-2. Runs 3+ parallel search strategies to find potential duplicates (only older issues)
-3. Verifies candidates by reading each one
-4. Posts a structured comment on the issue listing 1-3 confirmed duplicates (if any)
-
-**Skips:** closed issues, broad feedback issues, issues already checked
-
-**Related files:** [`scripts/comment-on-duplicates.sh`](scripts/comment-on-duplicates.sh)

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

@@ -3205,7 +3205,7 @@ public RelNode visitChart(Chart node, CalcitePlanContext context) {
         || node.getColumnSplit() == null
         || Objects.equals(config.limit, 0)) {
       // The output of chart is expected to be ordered by row split names
-      relBuilder.sort(relBuilder.field(0));
+      relBuilder.sort(relBuilder.nullsLast(relBuilder.field(0)));
       return relBuilder.peek();
     }
 
@@ -3275,7 +3275,8 @@ public RelNode visitChart(Chart node, CalcitePlanContext context) {
                 relBuilder.field(2))
             .as(aggFieldName));
     // The output of chart is expected to be ordered by row and column split names
-    relBuilder.sort(relBuilder.field(0), relBuilder.field(1));
+    relBuilder.sort(
+        relBuilder.nullsLast(relBuilder.field(0)), relBuilder.nullsLast(relBuilder.field(1)));
     return relBuilder.peek();
   }
 

core/src/main/java/org/opensearch/sql/expression/function/udf/binning/MinspanBucketFunction.java

@@ -14,6 +14,7 @@
 import org.apache.calcite.rex.RexCall;
 import org.apache.calcite.sql.type.ReturnTypes;
 import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
 import org.opensearch.sql.calcite.utils.PPLOperandTypes;
 import org.opensearch.sql.expression.function.ImplementorUDF;
 import org.opensearch.sql.expression.function.UDFOperandMetadata;
@@ -43,7 +44,7 @@ public MinspanBucketFunction() {
 
   @Override
   public SqlReturnTypeInference getReturnTypeInference() {
-    return ReturnTypes.VARCHAR_2000;
+    return ReturnTypes.VARCHAR_2000.andThen(SqlTypeTransforms.FORCE_NULLABLE);
   }
 
   @Override

core/src/main/java/org/opensearch/sql/expression/function/udf/binning/RangeBucketFunction.java

@@ -14,6 +14,7 @@
 import org.apache.calcite.rex.RexCall;
 import org.apache.calcite.sql.type.ReturnTypes;
 import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
 import org.opensearch.sql.calcite.utils.PPLOperandTypes;
 import org.opensearch.sql.expression.function.ImplementorUDF;
 import org.opensearch.sql.expression.function.UDFOperandMetadata;
@@ -47,7 +48,7 @@ public RangeBucketFunction() {
 
   @Override
   public SqlReturnTypeInference getReturnTypeInference() {
-    return ReturnTypes.VARCHAR_2000;
+    return ReturnTypes.VARCHAR_2000.andThen(SqlTypeTransforms.FORCE_NULLABLE);
   }
 
   @Override

core/src/main/java/org/opensearch/sql/expression/function/udf/binning/SpanBucketFunction.java

@@ -14,6 +14,7 @@
 import org.apache.calcite.rex.RexCall;
 import org.apache.calcite.sql.type.ReturnTypes;
 import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
 import org.opensearch.sql.calcite.utils.PPLOperandTypes;
 import org.opensearch.sql.expression.function.ImplementorUDF;
 import org.opensearch.sql.expression.function.UDFOperandMetadata;
@@ -41,7 +42,7 @@ public SpanBucketFunction() {
 
   @Override
   public SqlReturnTypeInference getReturnTypeInference() {
-    return ReturnTypes.VARCHAR_2000;
+    return ReturnTypes.VARCHAR_2000.andThen(SqlTypeTransforms.FORCE_NULLABLE);
   }
 
   @Override

integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java

@@ -25,6 +25,7 @@
   CalciteAddColTotalsCommandIT.class,
   CalciteConvertCommandIT.class,
   CalciteArrayFunctionIT.class,
+  CalciteBinChartNullIT.class,
   CalciteBinCommandIT.class,
   CalciteConvertTZFunctionIT.class,
   CalciteCsvFormatIT.class,