parthchandra commented on PR #7491: URL: https://github.com/apache/datafusion-comet/pull/7491#issuecomment-6809653154
> Thanks @parthchandra. The rework is much smaller, and sharing delete files by reference fixes the coalesce regression that comphead found. The merge in `IcebergPlanDataInjector.inject` is clean, and the SQL file test covers a good spread of partition values and layouts. > > I have one concern about residuals. A residual that applies to every file is now copied into each partition's message. The driver holds one copy per partition in `PartitionPlanData`, and each task ships one. In the release it was a single entry in the common block. With a large `IN` list residual and many partitions, the driver and wire cost grows with partitions times residual size. Did you measure a scan like that? If it matters, could residuals be interned once per scan and shared by reference the way `sharedDeleteFiles` is? The per-partition index would still point into the partition's own residual list, so the executor side would look much like the delete file path. > > I also have a question about the coalesce sharing. The serialization test in `PlanDataInjectorSuite` writes `CometExecPartition` objects directly, which looks right. Have you confirmed with a real `coalesce(n)` over several Iceberg partitions that the task size stays near one copy of a shared delete file? The SQL tests compare results, so they would not notice if the sharing were lost. Thanks Andy. On residuals, agreed. They're now interned once per scan and shared by reference, like delete files. `IcebergPartitionPlanData` carries them as shared arrays, and the executor appends them to the partition's residual pool, so residual_idx stays partition-local. The driver keeps one copy per scan and each task carries one, coalesced or not, which matches the release. I didn't measure a large IN-list scan before the change. The `PlanDataInjectorSuite` coalesce test now shares a 2,000-value IN residual and checks that 16 partitions serialize to under one partition plus the shared entries. On the coalesce check: the global equality-delete test in CometIcebergNativeSuite now runs a real coalesce(1). It serializes that task's partition with Spark's closure serializer, and asserts the delete file's path and bytes each appear once, with several partitions using the delete. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
