andygrove commented on PR #6130:
URL: 
https://github.com/apache/datafusion-comet/pull/6130#issuecomment-5876781464

   This is a light fully automated review since there are so many PRs open.
   
   Unlike its sibling operators, `CometArrowEvalPythonExec` 
(`spark/src/main/spark-4.1+/org/apache/spark/sql/comet/CometArrowEvalPythonExec.scala:152`)
 doesn't override `stringArgs`, `equals` or `hashCode`. Spark's default 
`stringArgs` is `productIterator`, so `argString` prints `nativeOp` through 
protobuf's `toString`, which dumps the whole native subtree. For 
`spark.read.parquet("s3a://...").select(f("x"))` that includes the pickled 
command bytes and the child scan's `object_store_options`, which 
`NativeConfig.extractObjectStoreOptions` fills with every `fs.s3a.*` key, 
`fs.s3a.secret.key` included. That text lands in `explain()`, the SQL UI and 
the event log, although `CometNativeScanExec` itself prints only `output`. The 
default `equals` and `hashCode` also compare `nativeOp`, whose `plan_id` 
differs between two otherwise identical subtrees, so an exchange above a native 
UDF can't be reused and a self-join runs the Python function once per side. 
Could this override the three metho
 ds the way `CometProjectExec` does, keeping the UDFs in the identity so two 
functions with the same result type still differ? A test asserting the plan 
string stays free of the command would guard against a regression.
   
   `docs/source/user-guide/latest/pyarrow-udfs.md:153` says PyArrow allocations 
are outside Comet's memory pool. They are also missing from the `allocated` 
figure in the executor's memory usage log, which counts only Rust's global 
allocator, even though the UDF output columns then flow through native 
operators that may reserve them. Sizing `spark.executor.memoryOverhead` from 
`allocated - reserved`, as the tuning guide describes, would come up short by 
whatever the interpreter and PyArrow hold. Could the guide say to add that on 
top, and could the non-Rust allocations list at 
`docs/source/contributor-guide/memory_management.md:364` mention the embedded 
interpreter and PyArrow's allocator?
   


-- 
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]

Reply via email to