andygrove commented on PR #5051: URL: https://github.com/apache/datafusion-comet/pull/5051#issuecomment-5147199985
@peterxcli @0lai0 thanks both — five of the six comments are addressed in 1367665bd, and two were real bugs. **Pruning config was silently ignored** (@peterxcli). `CometInMemoryTableScanExec` applied the stats filter unconditionally, so `spark.sql.inMemoryColumnarStorage.partitionPruning=false` did nothing. Now gated the same way Spark's `filteredCachedBatches` is. On your "not sure enabling pruning is always better" — pruning is normally the win, but the point is that the config exists so a user *can* turn it off, usually to rule out a stats bug when results look wrong; ignoring it breaks the one knob they are reaching for. **Readers leaked on early termination** (@peterxcli). Confirmed: `ArrowReaderIterator.close()` is only reached on exhaustion, so LIMIT/`take()`/cancellation left a reader open. Now registers a `TaskCompletionListener` as Spark's implementation does. Since `flatMap` consumes inner iterators sequentially, tracking the current reader is enough, and `close()` is already idempotent and synchronized. **Root leaked if conversion threw** (@0lai0). Fixed with the `try`/`NonFatal` guard you suggested. You are right the normal path cannot reach it, but `writeColumns` walks arbitrary `ColumnVector` implementations — the new path in this PR — so a misbehaving external vector is exactly the case. `rowToArrowBatchIter` had the same shape and got the same guard. **DISK_ONLY test** added to your spec: `memSize == 0`, `diskSize > 0`, all partitions cached, payload still `CometCachedBatch`, second query correct through the native scan. Your `Externalizable` reasoning held — it worked with no production change, which is exactly why the regression test is worth having. **`isArrowBacked` can be false** — that is the case this PR exists for: with `spark.comet.scan.enabled=false`, Spark's vectorized Parquet reader hands us `OnHeapColumnVector`s, and any external connector's `ColumnVector` does the same. The two "not Arrow-backed" tests at the end of the suite cover it. **AQE tests: not done.** Porting the three scenarios you linked is a real chunk of work on AQE plan-shape assertions rather than the cache format this PR changes, so I left it rather than rushing it. Since you said these could be follow-ups, I suggest a tracking issue for AQE coverage of `CometInMemoryTableScanExec` — I have not filed one, happy to if you want it. One thing worth passing on from writing the pruning test: reading a metric off `df.queryExecution.executedPlan` after `checkSparkAnswer(df)` gives **zero**, because `checkSparkAnswer` executes its own copies. My first version passed vacuously with 0 on both sides of the comparison; it needs the df forced explicitly. The real numbers are 100 rows decoded with pruning on versus 1000 with it off. All 19 tests in `CometInMemoryCacheSuite` pass on spark-3.5 (1 canceled, the pre-existing `isSpark40Plus` gate). -- 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]
