fix: ensure new_list respects data_type argument

[Ruchirtripathi]

Which issue does this PR close?

• Closes ScalarValue::new_list silently discards its data_type argument for non-empty input #24022

Rationale for this change

Fixes a bug where ScalarValue::new_list, new_list_nullable, and new_large_list silently ignored the
data_type argument when the values array was non-empty. This caused issues where accumulators like
collect_list could produce outputs with a slightly different type than declared (e.g., in the nullability of
nested fields), leading to invalid argument errors in GroupedHashAggregateStream::emit.

What changes are included in this PR?

• Added a cast_with_options call for non-empty lists in ScalarValue::new_list, new_list_from_iter, and
  new_large_list.
• Used DEFAULT_CAST_OPTIONS to ensure the concatenated array is properly reconciled with the requested
  data_type.

Are these changes tested?

Yes, this is covered by existing tests. It resolves the GroupedHashAggregateStream output batch validation
failures for accumulators.

Are there any user-facing changes?

No, this is an internal bug fix.

[neilconway]

@Ruchirtripathi Thanks for the contribution! Can you take a look at the test failures, please?

[Ruchirtripathi]

Hi! I've pushed a new commit that resolves the CI errors while maintaining the strict contract for #24022.
CI checks should be green now. @neilconway Let me know if everything looks good or if you need any further adjustments before merging!

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.85%. Comparing base (b902256) to head (7f0ef8a).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24029      +/-   ##
==========================================
- Coverage   80.86%   80.85%   -0.01%     
==========================================
  Files        1101     1101              
  Lines      375446   375466      +20     
  Branches   375446   375466      +20     
==========================================
- Hits       303598   303597       -1     
- Misses      53758    53775      +17     
- Partials    18090    18094       +4     

☔ 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]

@Ruchirtripathi,

Thanks for working on this. The constructor normalization looks like the right direction, but the DISTINCT and ordered ARRAY_AGG paths are still deriving their output types from runtime values. That can cause nested field nullability to differ from the aggregate's declared return and state schema.

I have left two blocking comments where the declared element types should continue to be used, along with a non-blocking suggestion to broaden the constructor regression coverage.

[Ruchirtripathi]

Thanks for the detailed feedback! I've gone ahead and addressed all the requested changes:

• Strict Type Enforcement in list constructors: ScalarValue::new_list, new_list_from_iter, and new_large_list now strictly enforce the provided DataType (using explicit casting) rather than inferring output types from runtime values. This preserves declared schemas, dictionaries, and
  nullability.
  * Array_agg Accumulator Fixes: The DistinctArrayAggAccumulator (and ordered paths) in array_agg.rs now strictly derive their output types
  from their declared return schema (&self.datatype) instead of derived runtime types, ensuring nested field nullability doesn't differ.
  • Test Corrections: The stricter schema enforcement in array_agg.rs revealed some existing tests that were improperly configuring the
    ArrayAggAccumulatorBuilder (declaring List(Utf8) but feeding Utf8 batches). I've corrected these test schemas to properly reflect their input
    data.
  • Expanded Regression Tests: Added regression tests in scalar/mod.rs to assert on inner array types for all three list constructors, and added
    ordered_aggregate_nested_nullability_mismatch_issue_24022 to explicitly test ordered aggregate nested nullability.

Let me know if there's anything else you'd like me to tweak @kosiew

[kosiew]

@Ruchirtripathi, thanks for the follow-up. The declared element types are now restored in both the DISTINCT and ordered ARRAY_AGG evaluation paths, and the new ordered aggregate regression looks good.

There is still one blocking gap: the DISTINCT accumulator path does not have equivalent nested-nullability regression coverage. Since this was the path where decoding could preserve different nested nullability, please add a test using a declared nullable struct field and a runtime non-nullable struct field. The test should assert the exact resulting list type and values.

I also left a non-blocking suggestion to expand the list constructor regression across all three constructors changed by this PR.

The targeted tests listed in the review passed, and I did not find any other correctness issues in the follow-up changes.

[kosiew]

Could we extend this regression to cover all three constructors changed by this PR?

Right now it only calls new_list and checks list.data_type(). A shared assertion for new_list, new_list_from_iter, and new_large_list would also let us verify both the declared nested child type and the normalized child values.

This is a suggestion only.

[kosiew]

Thanks for adding the ordered aggregate regression. Could you also add equivalent coverage for DistinctArrayAggAccumulator::evaluate?

Please use a declared struct type with a nullable field and runtime values whose struct field is non-nullable, then assert both the exact resulting list type and the values. This is the remaining blocking item because the DISTINCT decoding path can preserve different nested nullability.

[Ruchirtripathi]

Thanks for the detailed review! That makes sense. I'll add an equivalent regression for DistinctArrayAggAccumulator::evaluate using a declared struct type with a nullable field and runtime values whose struct field is non-nullable. I'll assert both the exact resulting list type and the values to cover the nested nullability preservation in the DISTINCT decoding path, then update the PR.

[Ruchirtripathi]

Hey! @kosiew Just pushed the latest changes.

I've added the missing nested-nullability regression test for the DISTINCT accumulator just like you asked. I set up a declared nullable struct field,
fed it a runtime non-nullable struct, and asserted that both the resulting list type and the exact inner values correctly preserved the declared
nullability. (I also went ahead and updated the ordered aggregate test to assert on the exact values as well, just to be thorough!)

As for the list constructor regressions for new_list, new_list_from_iter, and new_large_list, those were actually included in my previous
commit inside scalar/mod.rs, so we should be fully covered there.

Could you take a quick look and let me know if everything looks good to go now? Thanks again for the help!