Skip to content

Remove low priority iceberg it test cases to save test time [skip ci] - #13922

Merged
liurenjie1024 merged 4 commits into
NVIDIA:release/25.12from
liurenjie1024:ray/13881
Dec 2, 2025
Merged

Remove low priority iceberg it test cases to save test time [skip ci]#13922
liurenjie1024 merged 4 commits into
NVIDIA:release/25.12from
liurenjie1024:ray/13881

Conversation

@liurenjie1024

@liurenjie1024 liurenjie1024 commented Dec 2, 2025

Copy link
Copy Markdown
Collaborator

Related to #13881 #13885

Description

We removed a lot of low priority tests cases to reduce iceberg integration tests time. On my local desktop, its time reduced from 5.5h to 1.33h.

Checklists

  • This PR has added documentation for new or modified features or behaviors.
  • This PR has added new tests or modified existing tests to cover new code paths.
    (Please explain in the PR description how the new code paths are tested, such as names of the new/existing tests that cover them.)
  • Performance testing has been performed and its results are added in the PR description. Or, an issue has been filed with a link in the PR description.

Signed-off-by: liurenjie1024 <liurenjie2008@gmail.com>
Signed-off-by: liurenjie1024 <liurenjie2008@gmail.com>
Signed-off-by: liurenjie1024 <liurenjie2008@gmail.com>
@liurenjie1024 liurenjie1024 changed the title Ray/13881 Remove low priority iceberg it test cases to save test time. Dec 2, 2025
@greptile-apps

greptile-apps Bot commented Dec 2, 2025

Copy link
Copy Markdown
Contributor

Greptile Overview

Greptile Summary

This PR addresses timeout issues in Iceberg integration tests by reducing test permutations from an estimated 5.5 hours to 1.5 hours (73% reduction).

Key Changes:

  • Removed format_version parameter - now only tests with format version "2" (the current default)
  • Removed write_distribution_mode parameter (none, hash, range) - uses default mode
  • Removed fanout parameter (True, False) - uses default setting
  • Removed reader_type parameter (PERFILE, MULTITHREADED, COALESCING) - uses default COALESCING mode
  • Created new iceberg_overwrite_dynamic_test.py splitting dynamic overwrite tests from static

Impact Analysis:

  • Tests reduced from combinatorial explosion (e.g., 2 formats × 3 distributions × 2 fanouts × 3 readers = 36x) to single configuration
  • All removed parameters test non-default configurations that are lower priority for CI validation
  • Core functional coverage maintained: partition types, file formats, fallback scenarios, and both copy-on-write and merge-on-read modes are still tested
  • Format version 1 is legacy - focusing on version 2 aligns with current usage patterns

Confidence Score: 4/5

  • Safe to merge with minor considerations about test coverage trade-offs
  • The PR effectively solves the timeout problem through systematic test reduction while maintaining core functional coverage. Score is 4 (not 5) because: (1) removing format version 1 tests eliminates validation for legacy Iceberg tables, which some users may still have in production, and (2) removing reader_type parameters means PERFILE and MULTITHREADED reader modes are no longer validated in CI for these operations. However, these are acceptable trade-offs given the 73% time reduction and the fact that the removed configurations are lower priority.
  • No files require special attention - all changes follow a consistent pattern of parameter removal

Important Files Changed

File Analysis

Filename Score Overview
integration_tests/src/main/python/iceberg/iceberg_append_test.py 5/5 Removed format_version, write_distribution_mode, fanout, and reader_type test parameters, standardizing on format version 2 with default settings to reduce test permutations
integration_tests/src/main/python/iceberg/iceberg_ctas_test.py 5/5 Removed format_version, write_distribution_mode, and fanout test parameters, standardizing on format version 2 to reduce CTAS test permutations
integration_tests/src/main/python/iceberg/iceberg_delete_test.py 5/5 Removed reader_type parameter and associated configuration, simplifying DELETE operation tests while maintaining copy-on-write and merge-on-read coverage
integration_tests/src/main/python/iceberg/iceberg_merge_test.py 5/5 Removed reader_type parameter from all MERGE tests, reducing test permutations while preserving functional coverage
integration_tests/src/main/python/iceberg/iceberg_overwrite_dynamic_test.py 5/5 New file created with dynamic partition overwrite tests (no prior test parameters removed, fresh implementation)
integration_tests/src/main/python/iceberg/iceberg_overwrite_static_test.py 5/5 Removed format_version, write_distribution_mode, and fanout parameters from static overwrite tests, standardizing on format version 2
integration_tests/src/main/python/iceberg/iceberg_rtas_test.py 5/5 Removed format_version, write_distribution_mode, and fanout parameters from RTAS (Replace Table As Select) tests, standardizing on format version 2
integration_tests/src/main/python/iceberg/iceberg_update_test.py 5/5 Removed reader_type parameter from UPDATE tests, simplifying test matrix while maintaining copy-on-write and merge-on-read coverage

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant PR as PR #13922
    participant Tests as Iceberg Integration Tests
    participant CI as CI Pipeline
    
    Note over Dev,CI: Problem: Tests timing out after 5+ hours
    
    Dev->>PR: Reduce test permutations
    
    rect rgb(240, 240, 255)
        Note over PR,Tests: Parameter Reduction Strategy
        PR->>Tests: Remove format_version param (1,2) → use "2" only
        PR->>Tests: Remove write_distribution_mode (none,hash,range) → use default
        PR->>Tests: Remove fanout param (True,False) → use default
        PR->>Tests: Remove reader_type (PERFILE,MULTITHREADED,COALESCING) → use default
    end
    
    rect rgb(255, 240, 240)
        Note over Tests: Test Reduction Impact
        Tests->>Tests: Append: 6x fewer test cases
        Tests->>Tests: CTAS: 6-12x fewer test cases
        Tests->>Tests: Delete/Update/Merge: 3x fewer test cases
        Tests->>Tests: RTAS: 6-12x fewer test cases
        Tests->>Tests: Overwrite: 6-12x fewer test cases
    end
    
    Tests->>CI: Run reduced test suite
    
    rect rgb(240, 255, 240)
        Note over CI: Expected Result
        CI->>CI: 5.5h → 1.5h (73% reduction)
        CI-->>Dev: Tests complete within timeout
    end
Loading

@greptile-apps greptile-apps Bot 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.

8 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@liurenjie1024

Copy link
Copy Markdown
Collaborator Author

build

@res-life res-life left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@liurenjie1024
liurenjie1024 requested a review from pxLi December 2, 2025 07:23
@liurenjie1024 liurenjie1024 changed the title Remove low priority iceberg it test cases to save test time. Remove low priority iceberg it test cases to save test time [skip ci] Dec 2, 2025
@pxLi

pxLi commented Dec 2, 2025

Copy link
Copy Markdown
Member

also cc @yinqingh to help verify if we need some extra job or we can have this within DEFAULT mode for selected shims, thanks

@pxLi
pxLi requested a review from yinqingh December 2, 2025 07:28
@pxLi pxLi added the test Only impacts tests label Dec 2, 2025
@liurenjie1024

Copy link
Copy Markdown
Collaborator Author

build

@yinqingh

yinqingh commented Dec 2, 2025

Copy link
Copy Markdown
Collaborator

Confirmed with @liurenjie1024 , I will follow up and create a dedicate job to run ICEBERG_ONLY for 355

@liurenjie1024
liurenjie1024 merged commit eafe1f3 into NVIDIA:release/25.12 Dec 2, 2025
68 checks passed
@liurenjie1024

Copy link
Copy Markdown
Collaborator Author

Let's wait to see tomorrow's test run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Only impacts tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants