Upgrade DataFusion fork to 55.0.0 and preserve fork patches - #73

This is the one I would most like to revisit before merge.

branch-55 already carries this field from upstream, identically: config.rs:1294 has pub max_in_list_size: usize, default = 20, and this same proto file has uint64 max_in_list_size = 38; at line 620. Upstream main has both at the same tag with the same default. So the X-3375 backport has been subsumed upstream, and there is no fork patch left to preserve here.

What this hunk does is take upstream's plain field and turn it into a fork-only oneof. That is a new divergence rather than a preserved one, and it lands in the proto schema, where the cost is highest: our nodes now interpret an absent field differently from any upstream-based DataFusion, and every future upgrade has to re-apply the patch.

The stated motivation, that plans predating the field should not read as 0 and silently disable IN-list pruning, is a real concern. But it is equally a concern upstream, and if it is worth fixing it seems worth fixing there so everyone benefits and we stop carrying it.

The practical exposure also looks small: plans are serialized per RemoteExec request rather than persisted, branch-54 has carried the field since X-3375 so every running node writes a value, and reaching the absent case needs a node older than that backport in the rolling window.

Could this hunk go back to upstream's version? Dropping it also removes the two issues I flagged below, since both come from this change.

Also a wording point on the PR description: this is listed under "Preserves ... max_in_list_size protobuf semantics", but upstream's semantics are the plain field. This changes them.

Agreed. 7c67bad drops the provisional oneof; the schema, converters, and generated code now follow the branch-55 scalar field 38 representation. I also updated the PR accounting so X-3375 is Upstreamed rather than Reimplemented.

Two things here, both of which go away if the oneof above is reverted to upstream's plain field.

First, 20 is duplicated from config.rs:1294 (default = 20). If that default ever moves, this path silently keeps the old value, and the new test_parquet_options_max_in_list_size_absent_uses_default asserts the literal 20 too, so it would not catch the drift. ParquetOptions::default().max_in_list_size would keep the two tied together.

Second, the presence change is not symmetric across versions. In proto3 a plain scalar has implicit presence, so a writer on the old schema with max_in_list_size = 0 emits nothing for tag 38, which is byte-identical to never having set it. A reader on this branch maps that absence to 20. So during a rolling deploy an old node that deliberately set 0 to disable IN-list pruning has that decision read back as 20, which enables it.

Only that one direction is affected: new writers always emit the tag, so old readers are fine, and any non-zero value round-trips either way. It is not a correctness issue, only which row groups get read. It is also genuinely unrecoverable information rather than something this PR lost, since the two cases were already indistinguishable on the wire. Worth a line in the description about the upgrade window if the oneof stays.

Removed with 7c67bad. The duplicated default and asymmetric adapter disappear because both conversion paths now use the upstream scalar field directly.

take_all used to pick the output array type from self.data_type, the type the table was constructed with. It now picks it from self.owned, which set_batch overwrites with whatever array arrives.

Those agree as long as every batch carries exactly the declared string variant, which should hold. What changes is the failure mode if it ever does not: previously extract_string_value would have hit as_string::<i32>() on the wrong variant and panicked loudly, whereas now the table quietly adapts and take_all emits an array whose type may not match the plan's output schema.

Same pattern for the primitive table at line 312, where self.kt.clone() became self.owned.data_type().clone(). That one worries me slightly more because the parameterised types carry metadata: Decimal128(p, s) and Timestamp(unit, tz) would follow the last batch rather than the declared key type.

I could not construct a case where a batch actually disagrees with the declared type, so this may be entirely theoretical. But since the fields that pinned the intent (data_type, kt) were removed as part of the change, is it worth keeping one of them and asserting the batch matches in set_batch, so a mismatch stays loud?

Fixed in 00951ea. Both hash-table implementations retain the declared DataType, set_batch asserts an exact type match, and take_all emits the declared type. New regressions cover Utf8 versus LargeUtf8 and Timestamp timezone metadata; all 35 focused Top-K tests pass.

Small consistency point. This is a process-global counter, while the probe added in equivalence/properties/mod.rs deliberately uses a thread_local with the comment that "the test binary runs tests in parallel, and a shared count would let one test observe another's hits".

Only map_expressions_does_not_rebuild_expressionless_extension touches REBUILD_COUNT today and it stores 0 first, so nothing races right now. But the second test that uses it would race, and the same PR argues against exactly this shape a few files over. Worth matching the thread_local approach here.

Fixed in 0437d4f by removing the shared counter entirely. Arc::ptr_eq already proves that the extension node was not rebuilt, since rebuilding would allocate a different Arc, so the test no longer needs shared probe state.