viirya commented on issue #7091: URL: https://github.com/apache/datafusion-comet/issues/7091#issuecomment-6240030340
A few things I checked against main (DataFusion 55.2) that may help whoever picks this up. **Following the current advice can panic, not just do nothing.** With `spark.comet.exec.respectDataFusionConfigs` off, `spark.comet.datafusion.runtime.max_temp_directory_size` is dropped on the JVM side and has no effect. With the flag on, it reaches `SessionConfig::set_str` as `datafusion.runtime.max_temp_directory_size`. `set_str` unwraps the result of `ConfigOptions::set`, which rejects the unknown `runtime` namespace, so plan creation panics. Either way, the message sends users nowhere useful. **Where to add the hint.** I'd add it where the JVM exception is built in `native/jni-bridge/src/errors.rs`, rather than in `From<DataFusionError> for CometError`: - That `From` impl runs on many internal conversions. - `From<CometError> for DataFusionError` returns `source` and drops `msg`, so a hint added there is lost whenever the error goes through a round trip. - It would also leave `CometError::DataFusion.msg` different from `source.message()`. `natural_exception` is the point that both the plain `DataFusion` arm and the `DataFusionError::External` fallback reach. `generic_exception` alone would miss the `External` case. `generic_exception` already matches message text for file-not-found and duplicate-field errors, so there is precedent. Matching on the key `` `datafusion.runtime.max_temp_directory_size` `` seems the most robust choice, since that key is exactly the misleading part. **Hint wording.** The DataFusion sentence stays in the message, so the hint should say plainly that `datafusion.runtime.*` does not apply in Comet. Otherwise the two sentences contradict each other. It should also name `spark.comet.maxTempDirectorySize`, say that the limit applies to each native plan, and say that an executor can use several times that much. **Test against the real error, not a copied string.** The test should produce the error from a real `DiskManager` with a tiny limit (like the test in #7066 that writes past a 10-byte limit), then assert the hint shows up in the JVM message. If the test copies DataFusion's text, a wording change in a future upgrade would silently disable the hint and the test would still pass. A Rust unit test should be enough. **Interaction with #7066.** After #7066, the error still comes from the inner `DiskManager`'s `FileSpillWriter`, with the same text and the same path, so this works on either side of it. Wrapping the spill writer in `LazyDiskManager` to get a typed error is possible. But the wrapper would still need the message text, or a recount of `used_disk_space`, to tell this error apart from other I/O errors, so I don't think it's worth the extra code. Longer term, it would be cleaner for DataFusion to return a typed error here, or to let the caller supply the hint. That could be a separate upstream issue, and it shouldn't block this one. -- 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]
