feat: disable DataFrame.cache() for ballista #1420
Conversation
milenkovicm
left a comment
milenkovicm
left a comment
There was a problem hiding this comment.
thanks @killzoner
I have one suggestion
| } | ||
| } | ||
|
|
||
| impl CacheFactory for BallistaCacheFactory { |
There was a problem hiding this comment.
can we make cache factory return a logical plan extension, something like https://github.com/apache/datafusion/blob/20d5f06a554f39407e52dd1d4848267dbc1467ce/datafusion-examples/examples/dataframe/cache_factory.rs#L103
but with two additional properties, session_id and cache_id (populated from random id), we can then serialize it and send it to scheduler. Scheduler will then fail as there is no implementation. So when users want to implement cache they just need to add new query planner
There was a problem hiding this comment.
Ah i see the link clearer with #1395 (comment) now.
Didn't know about this behaviour with the scheduler, will look into this, thanks !
|
no need to implement anything on scheduler side at the moment |
2 participants
Which issue does this PR close?
Closes #1395 (partially if we want to expand on future configurable cache later).
Rationale for this change
Disable explicitly
cache()method with good user level error rather than relying on currentpanicWhat changes are included in this PR?
Add cache factory to properly error the unsupported feature (links to apache/datafusion#18893)
Are there any user-facing changes?
User level error instead of panic