Skip to content

HIVE-29598 follow-up: virtualize clearValue + replace qtest#1

Merged
ryukobayashi merged 1 commit into
ryukobayashi:HIVE-29598from
konstantinb:HIVE-29598-followup
May 19, 2026
Merged

HIVE-29598 follow-up: virtualize clearValue + replace qtest#1
ryukobayashi merged 1 commit into
ryukobayashi:HIVE-29598from
konstantinb:HIVE-29598-followup

Conversation

@konstantinb
Copy link
Copy Markdown

Follow-up to apache#6486 addressing two issues:

  1. Architecture (responds to @soumyakanti3578's review comment): the
    clearVectorValue private static instanceof-chain dispatch is replaced
    by a virtual ColumnVector.clearValue(int) method with a template-method
    clearSlotValue(int) hook. clearValue is final and centralizes the
    isNull[] / noNulls bookkeeping; subclasses override only clearSlotValue,
    so the contract cannot drift across future call sites or new subclasses.
    The operator's call sites collapse to one-liners. Additive to storage-api,
    no breaking changes.

  2. Test rigor: the original vector_outer_join7.q passes whether the
    fix is applied or not — it does not catch HIVE-29598. Confirmed by
    direct execution: pre-PR operator (97570b9cc2) + the author's
    original .q from 6b31424d46 produces output identical to the fixed
    operator. The CBO simplifies the plan (LEFT JOIN → ANTI JOIN, CASE WHEN
    → constant 'new') so aggressively that the null-padded columns are
    never read downstream.

    Replaced the .q with the actual repro from the HIVE-29598 description.
    New qtest:

    • Plan shows VectorMapJoinOuterMultiKeyOperator with
      smallTableValueMapping: 4:boolean — the slot reuse the JIRA predicts.
    • Output with fix: zero rows.
    • Output without fix: C 3 0 1 and D 3 0 1 leak through — exactly
      the rows the JIRA predicts.

Test plan

  • mvn test -pl storage-api — 26 tests pass (4 new JUnit 5 files for
    Long/Double/Decimal/IntervalDayTime + 2 extensions on existing JUnit 4
    Bytes/Timestamp).
  • mvn test -pl ql -Dtest=TestVectorMapJoinOuterGenerateResultOperator
    — 9 cases pass (3 individual covering Long-tracking + Void-baseline, plus
    6 parameterized over every modified ColumnVector subclass). Replaces the
    original PR's reflection-on-private-static test.
  • vector_outer_join7.q passes with fix; FAILS with exactly the 2-row
    diff under pre-PR operator. Both .q.outs captured for comparison.

Out of scope

The three isNull[i] = true writes in FULL OUTER paths (overflowBatch
writes at the original lines 771/880/1014) are deliberately unchanged. A
FULL OUTER repro attempt confirmed those paths do fire
(VectorMapJoinFullOuterLongOperator), but the FULL OUTER planner inserts
IfExprCondExprNull wrappers that respect isNull[] and recover from the
stale-value bug class. Without a working wrong-results repro, expanding
the fix there is speculative.


Drafted with Claude Code; verified and posted by @konstantinb.

Copy link
Copy Markdown
Owner

@ryukobayashi ryukobayashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I accept your proposal.

@ryukobayashi ryukobayashi merged commit eaf57ef into ryukobayashi:HIVE-29598 May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants