andygrove commented on PR #6048: URL: https://github.com/apache/datafusion-comet/pull/6048#issuecomment-5744441037
You're right, and the identity case is worse than the two from last round, because the transfer I added is what causes it. I reproduced your numbers: with a 4096-row int input the import allocator is left holding 128 bytes of the 16 KiB, matching your root = 17,302 / child = 128 exactly. **Taking the first option: the counters are allocator charges.** I went after the second one first and it isn't reachable without vendoring. `BaseAllocator.wrapForeignAllocation` routes through the same `allocateBytes` / `onAllocation` path as an ordinary `buffer()` call, and release goes through the same `releaseBytes` / `onRelease`, so an `AllocationListener` cannot distinguish foreign from JVM-created in either direction. Wrapping the `ForeignAllocation` that `ReferenceCountedArrowArray.unsafeAssociateAllocation` hands us would work, but `release0()` and `memoryAddress()` are both protected, so a delegating wrapper has to live in `org.apache.arrow.memory`. That is a worse trade than an honest contract for a diagnostic counter. So the tracing guide, the memory management guide and both scaladocs now say what the counters are: what each allocator is accountable for, not where the bytes were allocated, with the divergence spelled out in both directions and your `native_allocated` point stated as no guarantee rather than a caveat. The exact-subtraction claim is out of the PR description too. **I still fixed the identity case,** because the transfer is my code and it should not make attribution worse for a shape it was not aimed at. `result.getAllocator() != allocator` was a proxy for "the UDF allocated its output from the import allocator", and a returned input satisfies the proxy without being that case. The transfer now skips a result that is reference-identical to an input, and the input loop's close covers it. I deliberately did not chase the general case, since a result that merely shares buffers with an input, a slice say, still transfers. That one is named in the docs as an illustration of the contract rather than as a gap to close. **The regression you asked for** is in `CometUdfBridgeSuite`: an identity UDF, asserting the charge stays on the import allocator, that importing the export back reads 4096 correct values after the bridge's cleanup, and that the import allocator returns to its prior charge. Mutating the skip back out fails it at 128 of 16384 bytes, which is how I confirmed your measurement. Also ran `CometCodegenSuite` (98 tests, since it drives the bridge through real native execution), `CometScalaUDFClassLoaderSuite` and `NativeUtilSuite`, and cross-compiled against Spark 3.5 / Scala 2.12 as well as the default profile. -- 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]
