Fix in list round trip in df proto

[XiangpengHao]

Which issue does this PR close?

• Closes TPC-H Q16 fails during deserialization #16665.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Credits:

1. @ding-young for identifying the root cause, i.e., index should be applied on projection schema, not the full schema
2. Claude for fixing it

I looked at the patch and can't produce anything better than Claude

(add XiangpengHao/liquid-cache#293 for record)

[XiangpengHao]

Ok I run a bisect and find the code is broken since #15769, I guess it's because the filter is moved to a different sub-structure.

@adriangb can you take a look if this is the right way to fix it?

[NGA-TRAN]

Thanks @XiangpengHao for the fix. Could you also run the tests in this PR? The deserialization bug only happens to one tpc-h query (q16) but when I run roundtrip tests, only 4 out of 22 queries pass. Hopefully, this fix will also solve that

[adriangb]

@adriangb can you take a look if this is the right way to fix it?

I took an initial look and... I'm a bit stumped. I don't fully understand where this is running or how, I haven't looked too deeply into proto serialization of plans and I don't know if DataDog is doing that before or after optimizer rules are applied, etc.
What I'll say about #15769 is that it shouldn't have changed the structure of FilterExec. It just moves the filters from FilterExec into ParquetSource(possibly deleting the FilterExec). In other words: the plan is being rewritten in the optimizer pass, but I think the final state of the ExecutionPlan tree should not have changed in terms of serialization.

But I think I need to take another look at this.

I'll be completely AFK for the the weekend until Tuesday and have an early end to my day today so if I don't get back to give a deeper analysis for this I suggest that as long as:

1. This fixes the tests.
2. Does not break any other tests (especially Add serialization/deserialization and round-trip tests for all tpc-h queries #16742).

It's worth just proceeding with it and I can try to find a deeper meaning to it next week.

Might also be worth asking Claude to explain why this is the fix to give us pointers into what to look at?

[NGA-TRAN]

@XiangpengHao: If you believe the round-trip bug reproduced in test_round_trip_tpch_queries from PR #16742 is distinct, we can file a separate issue and tackle it independently. @LiaCastaneda is interested and may be available to work on a fix.

[XiangpengHao]

@XiangpengHao: If you believe the round-trip bug reproduced in test_round_trip_tpch_queries from PR #16742 is distinct, we can file a separate issue and tackle it independently. @LiaCastaneda is interested and may be available to work on a fix.

I think it's a different bug, see #16742 (comment)

@LiaCastaneda feel free to fix it!

[XiangpengHao]

@adriangb can you take a look if this is the right way to fix it?

I took an initial look and... I'm a bit stumped. I don't fully understand where this is running or how, I haven't looked too deeply into proto serialization of plans and I don't know if DataDog is doing that before or after optimizer rules are applied, etc. What I'll say about #15769 is that it shouldn't have changed the structure of FilterExec. It just moves the filters from FilterExec into ParquetSource (possibly deleting the FilterExec). In other words: the plan is being rewritten in the optimizer pass, but I think the final state of the ExecutionPlan tree should not have changed in terms of serialization.

But I think I need to take another look at this.

I'll be completely AFK for the the weekend until Tuesday and have an early end to my day today so if I don't get back to give a deeper analysis for this I suggest that as long as:

1. This fixes the tests.
2. Does not break any other tests (especially Add serialization/deserialization and round-trip tests for all tpc-h queries #16742).

It's worth just proceeding with it and I can try to find a deeper meaning to it next week.

Might also be worth asking Claude to explain why this is the fix to give us pointers into what to look at?

Agree! I think it's not that #15769 introduces the bug, it just surfaces up the bug. I plan to dig deeper into this over the weekend.

[alamb]

Suggested change

	// Failing due to https://github.com/apache/datafusion/pull/16662
	#[ignore]
	// Fixed: Column index mismatch during protobuf deserialization
	// Test case for https://github.com/apache/datafusion/pull/16662

[alamb]

Thank you @XiangpengHao

FYI @NGA-TRAN and @LiaCastaneda