Skip to content

Validate TPC-DS Q8 - #22473

Merged
rapids-bot[bot] merged 5 commits into
NVIDIA:mainfrom
Matt711:bug/pdsds/q8
May 13, 2026
Merged

Validate TPC-DS Q8#22473
rapids-bot[bot] merged 5 commits into
NVIDIA:mainfrom
Matt711:bug/pdsds/q8

Conversation

@Matt711

@Matt711 Matt711 commented May 12, 2026

Copy link
Copy Markdown
Member

Description

Workaround for #22317

The issue seems to be that we silently drop rows in multi-rank joins (described more in #22318 (comment)). This is why the sum changes non-deterministically from run-to-run. But the changes in that PR don't seem to be sufficient to validate Q8. Therefore, the change in this PR is a workaround until we figure where else we could be dropping rows.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@Matt711
Matt711 requested a review from a team as a code owner May 12, 2026 12:32
@Matt711
Matt711 requested a review from wence- May 12, 2026 12:32
@Matt711 Matt711 added bug Something isn't working non-breaking Non-breaking change labels May 12, 2026
@copy-pr-bot

copy-pr-bot Bot commented May 12, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added Python Affects Python cuDF API. cudf-polars Issues specific to cudf-polars labels May 12, 2026
@Matt711

Matt711 commented May 12, 2026

Copy link
Copy Markdown
Member Author

/ok to test d5290ac

@GPUtester GPUtester moved this to In Progress in cuDF Python May 12, 2026
@coderabbitai

coderabbitai Bot commented May 12, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Query 8 refactors ZIP prefix filtering logic to materialize prefix columns explicitly before joining. Instead of slicing ZIP strings inline in join keys, prefixes are computed upfront in intersect_zips and the main query, then used as materialized join columns to enable deterministic execution under the streaming executor.

Changes

ZIP prefix materialization for deterministic joins

Layer / File(s) Summary
Intersect ZIPs with materialized prefix
python/cudf_polars/cudf_polars/experimental/benchmarks/pdsds_queries/q8.py
intersect_zips now joins 5-character target and preferred ZIPs, then derives and materializes a 2-character ca_zip_prefix column as the intersection key.
Main query join using precomputed ZIP prefixes
python/cudf_polars/cudf_polars/experimental/benchmarks/pdsds_queries/q8.py
Main query derives s_zip_prefix via with_columns and switches the intersect_zips join from inline slice expressions to explicit prefix column matching (s_zip_prefixca_zip_prefix).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Validate TPC-DS Q8' is clear and specific, directly referring to the main change which is addressing validation issues for TPC-DS Query 8, a specific benchmark query mentioned throughout the PR objectives and linked issues.
Linked Issues check ✅ Passed The code changes address the linked issue #22317 by restructuring the join logic in Q8 to work around the silent row-dropping issue in multi-rank joins, implementing a workaround by materializing ZIP prefixes and using precomputed columns for joins instead of inline slice expressions.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the Q8 query implementation file, focusing on restructuring the join logic to address the validation issue described in issue #22317 with no unrelated modifications.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check ✅ Passed The pull request description clearly explains the changes as a workaround for a nondeterministic validation failure in TPC-DS Query 8, referencing specific GitHub issues and the root cause (silently dropped rows in multi-rank joins).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@Matt711

Matt711 commented May 12, 2026

Copy link
Copy Markdown
Member Author

/ok to test 401abca

@TomAugspurger TomAugspurger 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.

Looks fine for now, but maybe remove the "Closes #22317" from your PR description and we can use that as the issue to track work on identifying the root cause?

@Matt711

Matt711 commented May 13, 2026

Copy link
Copy Markdown
Member Author

/merge

@Matt711

Matt711 commented May 13, 2026

Copy link
Copy Markdown
Member Author

/ok to test ce0da68

@Matt711

Matt711 commented May 13, 2026

Copy link
Copy Markdown
Member Author

/ok to test 2c47d30

@rapids-bot
rapids-bot Bot merged commit cae25ae into NVIDIA:main May 13, 2026
90 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in cuDF Python May 13, 2026
@Matt711
Matt711 deleted the bug/pdsds/q8 branch May 13, 2026 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cudf-polars Issues specific to cudf-polars non-breaking Non-breaking change Python Affects Python cuDF API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants