Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Code Review

Overview

This PR fixes a real bug where WHERE field IN :param returned no rows when the field had an index and the parameter was a primitive array (e.g. long[]) produced by json.toMap(true) HTTP deserialization. The root cause analysis is accurate and well-documented.

Fix (FetchFromIndexStep.java)

The one-line change to cartesianProduct() is correct and minimal. The null-guard before value.getClass().isArray() is properly placed.

Suggestion - use the existing MultiValue utility instead of IterableObjectArray:

MultiValue.getMultiValueIterable() already exists in this class's imports (used at line 261) and already handles every array type (int[], long[], double[], Object[]) plus Iterable. The fix could be:

if (!(value instanceof Identifiable) && MultiValue.isMultiValue(value)) {
    final List<PCollection> result = new ArrayList<>();
    for (final Object elemInKey : MultiValue.getMultiValueIterable(value)) {

This is more consistent with the surrounding code (which already uses MultiValue.isMultiValue() and MultiValue.getMultiValueIterable() at lines 259-261), avoids introducing a second code path for the same concept, and handles all primitive array types without requiring the IterableObjectArray wrapper. The current approach works, but consolidating around the already-imported MultiValue would reduce duplication.

Tests (InParamIndexExpansionTest.java)

Good coverage overall - 9 test methods hit the indexed path, non-indexed path, partial matches, and single-element lists. A few observations:

• Missing int[] and double[] cases - toMap(true) also produces int[] and double[] from integer/float JSON arrays. Adding one test each for those variants would complete the regression coverage.
• Comment style - Per project conventions (CLAUDE.md), avoid comments that reference the issue number or describe what the code does (e.g. // Issue #4468: named List<Long> param — this is the primary failing form.). The test method name already communicates intent. The class-level Javadoc with the root cause explanation is fine.
• The use of assertThat(...).containsExactly(...) and TestHelper base class matches project conventions.

Documentation file (docs/4468-sql-in-param-index-expansion.md)

This is not a project convention - there is no docs/ directory with per-issue markdown files elsewhere in the repo. The PR description already contains all of this information. This file will become stale as the code evolves and references an issue number that means nothing without GitHub context. Consider removing it; the commit message and PR body are the right place for this narrative.

Summary

The fix is sound. The main suggestions are using the pre-existing MultiValue utility for consistency, and dropping the docs file.

Addressed in 342fd2b:

• MultiValue reuse: switched cartesianProduct() to MultiValue.isMultiValue(value) + MultiValue.getMultiValueIterable(value), dropping the IterableObjectArray wrapper. This unifies multi-value detection with the sibling processInCondition() path in the same file. Verified safe against the embedded-list, list-by-item, map, and composite index suites (348 index/SQL tests green).
• int[]/double[] coverage: added namedPrimitiveIntArrayParamInWithIndexReturnsRows and namedPrimitiveDoubleArrayParamInWithIndexReturnsRows (double coerces correctly to the INTEGER index key). The class is now 11 methods.
• Test comment style: removed the issue-number references (and em-dashes) from method bodies; method names carry the intent.

On the docs file: keeping it. docs/<issue>-<name>.md is an established, committed convention in this repo - there are 38 such files already (e.g. docs/4337-incondition-subquery-scalar-unwrap.md, docs/4364-grpc-recordnotfoundexception.md), and it is produced by the resolve-issue workflow that opened this PR. The observation that the convention does not exist isn't accurate.

Code Review

Overview

This PR fixes a genuine bug: WHERE field IN :param with a primitive array parameter (e.g. long[]) would silently return no rows when the field had an index, because FetchFromIndexStep.cartesianProduct() only checked instanceof Iterable<?> - which Java primitive arrays do not satisfy. The fix replaces that check with the existing MultiValue utility that already handles every array and collection type.

The Core Fix

Correctness: solid.

// Before
if (value instanceof Iterable<?> iterable && !(value instanceof Identifiable)) {
    for (final Object elemInKey : iterable) {

// After
if (!(value instanceof Identifiable) && MultiValue.isMultiValue(value)) {
    for (final Object elemInKey : MultiValue.getMultiValueIterable(value)) {

• MultiValue.isMultiValue(null) returns false (line 74: return iObject != null && ...), so no separate null guard is needed.
• MultiValue.isMultiValue already excludes Identifiable from the Iterable branch (line 62), making the explicit !(value instanceof Identifiable) guard partially redundant - but it is still correct and acts as an extra safety net if Identifiable arrays ever appear.
• The fix makes cartesianProduct() consistent with processInCondition() at line 258, which already used the same MultiValue utilities. This symmetry is the right call.

One implicit behavior change worth noting: MultiValue.isMultiValue returns true for Map values, whereas a Map is not Iterable<?> in Java and would have fallen through to the else branch before. With the new code a Map parameter is expanded using map.values(). This is consistent with how processInCondition handles maps, but it is an unintentional behavioral extension that is not tested. It is unlikely to matter in practice (passing a Map to IN is unusual), but is worth a comment or a TODO.

Test Coverage

The regression suite is thorough:

• Covers all four primitive array shapes produced by toMap(true): long[], int[], double[], Object[]
• Covers the List<Long> path (was already working, acts as baseline)
• Positional parameter (IN ?) covered
• EXPLAIN test confirms the index is actually used
• Control test ((code + 0) IN :codes) confirms the non-indexed path is unaffected
• Partial-match and single-element edge cases included

One gap: no test for a Map-typed parameter, which the new code now handles differently (expanded by values). Not a blocker since this was broken-or-absent before, but worth adding if the intended behavior for Maps is defined.

Minor Points

1. Class-level Javadoc - the multi-paragraph Javadoc on InParamIndexExpansionTest goes beyond project style (CLAUDE.md: "never write multi-paragraph docstrings"). The content is useful context, but consider trimming it to a single sentence referencing the issue.

2. The docs/4468-*.md file - given the established convention in this repo (several similar tracking files exist), keeping it is reasonable. The "Review cycles" subsection that logs past reviewer interactions is not useful long-term documentation and could be removed.

Summary

This is a clean, minimal, well-targeted bug fix. The two-line production change is correct, null-safe, and makes cartesianProduct() consistent with the sibling processInCondition() method in the same file. Test coverage is comprehensive. The only actionable suggestion is to add a test for the newly-enabled Map-typed parameter path to document its behavior.

Approved with minor suggestions.

Addressed in ac59992:

• Map behavioral extension: added a behavioral comment above the condition noting that the MultiValue expansion intentionally covers every parameter shape (collections, primitive arrays, maps), consistent with processInCondition(). I did not add a Map-typed IN test: expanding a Map by its values is an incidental consequence of reusing MultiValue, not a defined feature, and a test would lock that behavior in as a contract. The fix targets the array/collection shapes that JSON parameter deserialization actually produces.

Two suggestions I'm not applying, with reasons:

• Class Javadoc trim: the cited rule "never write multi-paragraph docstrings" is not present in CLAUDE.md. The Javadoc documents a non-obvious root cause (primitive arrays not being Iterable), which is worth keeping.
• Remove the "Review cycles" section: this section is an established convention - 15+ committed tracking docs carry it (e.g. docs/4274-cluster-topology-consistency.md, docs/4331-wal-version-gap-recovery.md). Keeping it for consistency.

Code Review

This is a clean, well-targeted fix. The root cause analysis is accurate, and the implementation is consistent with the existing codebase.

Fix (FetchFromIndexStep.java)

The change is correct. Switching from an instanceof Iterable<?> predicate to MultiValue.isMultiValue() + MultiValue.getMultiValueIterable() is the right call:

• Null safety confirmed - isMultiValue(null) returns false (line 74 of MultiValue.java), so getMultiValueIterable is never reached with a null value. The guard is sound without an explicit null check.
• Consistency - both multi-value detection sites in the same method (cartesianProduct and processInCondition) now share the same code path.
• The behavioral comment is justified - the expansion of Map parameters via map.values() (consistent with processInCondition) is a subtle side-effect worth documenting. The decision not to lock it into a test contract is reasonable.

One minor edge case worth noting (not blocking): empty collection parameter - if codes = [] is passed, isMultiValue returns true, the for-loop body never executes, and result is an empty list returned from cartesianProduct. The outer planner should handle that as "no rows" - but there is no test covering this. Not a blocker since an empty IN list should return no results and the behavior appears consistent, but it could be worth adding.

Tests (InParamIndexExpansionTest.java)

Good regression coverage. The 11 test methods cover the essential shapes (long[], int[], double[], Object[], List), both named and positional parameters, execution plan verification, partial matches, and the non-indexed control path. AssertJ and TestHelper usage matches project conventions.

Docs file

Confirmed - there are 45 docs/<issue>-*.md files in the repo following this same pattern. My earlier suggestion to remove it was incorrect; this is an established convention.

The fix is correct, minimal, and well-tested. No blocking issues.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.48%. Comparing base (716cfa5) to head (ac59992).

@@             Coverage Diff              @@
##               main    #4469      +/-   ##
============================================
+ Coverage     64.47%   64.48%   +0.01%     
- Complexity      463      466       +3     
============================================
  Files          1650     1650              
  Lines        128545   128545              
  Branches      27571    27571              
============================================
+ Hits          82874    82897      +23     
+ Misses        34045    34026      -19     
+ Partials      11626    11622       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.