Skip to content

Unify map() list-row handling across list representations#23859

Open
kosiew wants to merge 3 commits into
apache:mainfrom
kosiew:unify-list-01-22986
Open

Unify map() list-row handling across list representations#23859
kosiew wants to merge 3 commits into
apache:mainfrom
kosiew:unify-list-01-22986

Conversation

@kosiew

@kosiew kosiew commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

map() had separate decomposition and null-row filtering paths for List/LargeList and FixedSizeList, resulting in duplicated logic and making it harder to verify equivalent behavior across supported outer list representations.

This change introduces shared cross-representation tests and consolidates the internal row access and decomposition logic behind a single private adapter while preserving existing behavior and error handling.

What changes are included in this PR?

  • Introduce a private ListArrayRepresentation adapter to provide a common row-oriented interface for List, LargeList, and FixedSizeList.

  • Route map decomposition through the shared adapter instead of maintaining separate constructor paths for variable-sized and fixed-size lists.

  • Reuse the same key-driven null-row filtering logic for both key and value rows.

  • Route key validation through the shared row adapter.

  • Remove duplicated helper functions and the dedicated fixed-size list construction path that are superseded by the unified implementation.

  • Add reusable test fixtures and assertions for validating complete MapArray outputs.

  • Add cross-representation tests verifying equivalent behavior for:

    • null rows interleaved with non-null rows,
    • sliced inputs containing null rows,
    • all-null outer rows.
  • Verify map offsets, outer validity bitmap, and child key/value arrays are identical across supported list representations.

Are these changes tested?

Yes.

The following tests were added:

  • test_make_map_null_rows_are_equivalent_across_list_representations
  • test_make_map_sliced_null_rows_are_equivalent_across_list_representations
  • test_make_map_all_null_rows_are_equivalent_across_list_representations

These complement the existing map tests by asserting equivalent behavior for List, LargeList, and FixedSizeList representations without changing expected outputs.

Are there any user-facing changes?

No. This is an internal refactoring and test expansion intended to preserve existing behavior and error messages while reducing duplicated implementation code.

LLM-generated code disclosure

This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.

kosiew added 3 commits July 24, 2026 19:45
- Separate list representation from borrowed arrays
- Reuse key-derived representation for keys and values
- Clarify private names and comments
- Deduplicate test representation matrix
- Leave boxed iterator and semantic-risk items unchanged
- Added tests for List, LargeList, and FixedSizeList.
- Utilized non-zero outer slice offsets.
- Covered internal null key row and hidden value row.
- Reused assertions for full offsets, nulls, and children.
- No changes made to production code.
@github-actions github-actions Bot added the functions Changes to functions implementation label Jul 24, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.19101% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.71%. Comparing base (18b1e35) to head (1f84fcb).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/functions-nested/src/map.rs 97.19% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #23859    +/-   ##
========================================
  Coverage   80.70%   80.71%            
========================================
  Files        1090     1090            
  Lines      370150   370371   +221     
  Branches   370150   370371   +221     
========================================
+ Hits       298746   298960   +214     
+ Misses      53598    53596     -2     
- Partials    17806    17815     +9     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kosiew
kosiew marked this pull request as ready for review July 24, 2026 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants