andygrove commented on PR #5051: URL: https://github.com/apache/datafusion-comet/pull/5051#issuecomment-5432632450
@sunchao thanks, that is a genuinely valuable review and all five findings were real. Fixed in cb07053e3, with a regression test for each. Replies are on the individual threads; the short version: | Finding | Status | Introduced by this PR? | |---|---|---| | Join over empty-output cache scan returns wrong results | Fixed | No, predates it | | Dictionary provider reused across columns | Fixed | **Yes** | | Readers leak when a later column fails to open | Fixed | **Yes** | | Cache RDD built during planning | Fixed | No, predates it | | Large-offset Arrow vectors accepted then rejected | Fixed | No, predates it | The P1 fix is better than a repair. The code widened an empty projection to a placeholder column because a comment asserted the native plan needs a non-empty scan schema. That assertion is simply false: passing `op.output` through unchanged, empty and all, plans and runs fine. So the widening is deleted rather than reconciled with `output`, which fixes your join **and** makes `count(*)` decode nothing at all, which is better than the placeholder column I had introduced a commit earlier. Two of the five are mine, both consequences of splitting the payload into one stream per column: independent readers mean independent dictionary namespaces, and a partial failure mid-construction has no owner yet. Worth saying plainly, since the split is a change I made deliberately and these are its costs. One process note in case it is useful to you. My first version of the planning regression test asserted on `isCachedColumnBuffersLoaded`, which also requires the blocks to be populated, so it stayed false either way and passed with the fix reverted. I only caught it because I revert each fix and re-run before claiming a test works. The version on the branch asserts the cached plan still reports `isFinalPlan=false` after `explain()`, which is the effect you actually described, and it does fail without the fix. All 30 tests in `CometInMemoryCacheSuite` pass, plus `UtilsSuite`, `CometNativeColumnarToRowSuite` and `NativeUtilSuite`. Scalafix is clean on the 3.4, 3.5 and 4.0 profiles. Separately, I went through the Spark work you linked and filed #5487 with the comparison, including a couple of things worth taking from it that are unrelated to these findings. -- 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]
