Skip to content

[VL] Add timestamp to shuffle test#12503

Merged
marin-ma merged 3 commits into
apache:mainfrom
marin-ma:add-timestamp-to-shuffle-test
Jul 14, 2026
Merged

[VL] Add timestamp to shuffle test#12503
marin-ma merged 3 commits into
apache:mainfrom
marin-ma:add-timestamp-to-shuffle-test

Conversation

@marin-ma

@marin-ma marin-ma commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Timestamp type is missing in the shuffle test. This patch adds timestamp type in the test data. Note that because spark only supports microsecond precision, the nanosecond field should be set to 0, otherwise the sort shuffle tests will fail.

Also add type check for the shuffle inputs.

Verified velox_gpu_shuffle_writer_test locally.

Copilot AI review requested due to automatic review settings July 13, 2026 15:51
@github-actions github-actions Bot added the VELOX label Jul 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Velox shuffle writer tests to include a Timestamp column (with nanoseconds set to 0 to match Spark’s microsecond precision) and tightens shuffle writer correctness by enforcing that all input RowVectors written through a writer instance have an equivalent RowType.

Changes:

  • Add facebook::velox::Timestamp columns to the shared shuffle test input vectors and introduce shared “has-null” variants in the test base.
  • Reuse shared “has-null” inputs in CPU shuffle writer tests instead of constructing ad-hoc vectors in the test body.
  • Add RowType equivalence checks in shuffle writers (and centralize rowType_ in VeloxShuffleWriter) to fail fast on schema mismatches across writes.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
cpp/velox/tests/VeloxShuffleWriterTestBase.h Adds timestamp test data and shared “has-null” input row vectors to be reused across tests.
cpp/velox/tests/VeloxShuffleWriterTest.cc Switches CPU tests to reuse shared “has-null” inputs from the test base.
cpp/velox/tests/VeloxGpuShuffleWriterTest.cc Adds timestamp column to locally constructed GPU test inputs (but currently still builds schema-mismatched vectors).
cpp/velox/shuffle/VeloxSortShuffleWriter.h Removes per-writer rowType_ member in favor of the base class field.
cpp/velox/shuffle/VeloxSortShuffleWriter.cc Initializes rowType_ via rv->rowType() and enforces row-type equivalence across writes.
cpp/velox/shuffle/VeloxShuffleWriter.h Adds shared rowType_ storage to the base shuffle writer class.
cpp/velox/shuffle/VeloxRssSortShuffleWriter.h Removes duplicate rowType_ member (now inherited from base).
cpp/velox/shuffle/VeloxRssSortShuffleWriter.cc Adds row-type equivalence checks on subsequent RowVector inputs.
cpp/velox/shuffle/VeloxHashShuffleWriter.cc Stores the writer’s rowType_ and enforces equivalence for subsequent inputs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cpp/velox/tests/VeloxGpuShuffleWriterTest.cc Outdated
Comment thread cpp/velox/tests/VeloxGpuShuffleWriterTest.cc Outdated
Copilot AI review requested due to automatic review settings July 13, 2026 17:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comment thread cpp/velox/tests/VeloxGpuShuffleWriterTest.cc Outdated
Comment thread cpp/velox/tests/VeloxGpuShuffleWriterTest.cc Outdated
@marin-ma marin-ma requested review from Copilot and removed request for Copilot July 14, 2026 09:19
@marin-ma marin-ma merged commit 14683bb into apache:main Jul 14, 2026
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants