andygrove commented on PR #6085: URL: https://github.com/apache/datafusion-comet/pull/6085#issuecomment-5778788334
The reasoning on dropping the planning force holds up. `postDriverMetricUpdates` returns without reading any metric value when the execution id is null, so the force really does disappear in that case, but `findAllPlanData` calls `ensureSubqueriesResolved()` immediately before and reads `commonData` immediately after, so the ordering survives on the fused path, and the standalone path needs the partition data anyway. One gap. The double-post guard is the substance of this change and I do not think anything fails if it is removed. The two new cases cover one call site each, and the paths are mutually exclusive for a given scan as you say, so neither posts twice. The count assertions are `.trim != "0"`, which a doubled value passes just as well as a correct one. You already have the number that would catch it, from the spark-shell run in the description: `resultDataFiles=16, not 32`. Could `assertPlanningMetricsInUi` assert the exact expected `resultDataFiles` and `totalDataManifest` for the fixture table instead of just non-zero? That turns the manual check into a regression test. Calling `scan.sendDriverMetrics()` a second time before reading the store would be the more direct version if the exact counts are awkward to pin down. Also worth a word in the comment that `postedExecutionId` is per scan instance. Two instances of the same scan under one execution id would still post twice, since `copy()` does not carry the field. Probably not reachable today, but the comment currently reads as though the whole execution is covered. -- 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]
