mixermt commented on PR #6106: URL: https://github.com/apache/datafusion-comet/pull/6106#issuecomment-5773386295
Review of the first revision found three problems, all addressed in the latest push: 1. **The description overclaimed for S3.** At the pinned iceberg-rust rev `OpenDalStorage::S3` holds only config and the access loader; `create_operator` builds a new opendal `Operator`, and with it a new `Signer`, on every file open, while HTTP pooling is already process-wide. A shared `FileIO` therefore reuses the factory, the parsed config and the JNI bridge, not the S3 client. Client reuse holds for the HDFS backend in #5898, whose `Storage` caches operators per NameNode. The description now states this per backend, and an operator cache for the S3 family is noted as an iceberg-rust follow-up in #6105. 2. **The bound cleared the whole map**, including the entry a running query was using, and dropped every `FileIO` while holding the lock. Replaced with LRU eviction of one entry, dropped after the lock is released. 3. **The test never exercised a cache hit.** Added tests that a repeated identical load builds once, that a load without a key builds every time, that `memory:///` never enters the cache, and that eviction removes the least recently used entry. -- 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]
