DataFusion Cache Support - #1
Conversation
alchemist51
left a comment
alchemist51
left a comment
There was a problem hiding this comment.
Gave it a initial pass! Will be digging more into it today!
|
|
||
| #[no_mangle] | ||
| pub extern "system" fn Java_org_opensearch_datafusion_DataFusionQueryJNI_createSessionContext( | ||
| pub extern "system" fn Java_org_opensearch_datafusion_DataFusionQueryJNI_createSessionContextv1( |
There was a problem hiding this comment.
we can still keep this createSessionContext ?
| if config_ptr != 0 { | ||
| let cache_manager_config = unsafe { &mut *(config_ptr as *mut CacheManagerConfig) }; | ||
| // This replaces the contents at the same pointer location | ||
| *cache_manager_config = cache_manager_config.clone() |
There was a problem hiding this comment.
Why do we need to clone?
| let store = Arc::new(object_store::local::LocalFileSystem::new()); | ||
|
|
||
| // Use Runtime to block on the async operation | ||
| let metadata = Runtime::new() |
There was a problem hiding this comment.
let's use the global runtime instead of creating our own here.
| return 0; // Skip unsupported formats | ||
| }; | ||
|
|
||
| let object_meta = create_object_meta_from_file(&file_path); |
There was a problem hiding this comment.
This can fail as well, ideally the function should have Result() return to capture the error.
| _class: JClass, | ||
| cache_ptr: jlong, | ||
| file_path: JString, | ||
| ) -> i32 { |
There was a problem hiding this comment.
We need to think of return type here. Are there any return codes we are following?
There was a problem hiding this comment.
I am thinking of having (1,0,-1) OR maybe Result() if we want to propagate error messages back to Java. Ideally we should have boolean to check if it was code other than 1, a pre-configured retry mechanism should be triggered
| _class: JClass, | ||
| cache_ptr: jlong, | ||
| file_path: JString, | ||
| ) -> jlong { |
There was a problem hiding this comment.
We are using bool, i32, jlong in different functions.. probably we need to come to a common way.
| Err(_) => return false | ||
| }; | ||
| let cache = unsafe { &*(cache_ptr as *const Arc<DefaultFilesMetadataCache>) }; | ||
| let object_meta = create_object_meta_from_file(&file_path); |
There was a problem hiding this comment.
Why are we creating object meta here?
There was a problem hiding this comment.
We are checking if a given key exists in the cache. As metadata_cache has object_meta as key, we are constructing the key here.
But i think we should rather pick this up from a static entity perhaps listing_table_cache which already has ObjectMeta stored corresponding to each filePath
| } No newline at end of file | ||
| } | ||
|
|
||
| pub fn create_object_meta_from_file(file_path: &str) -> ObjectMeta { |
There was a problem hiding this comment.
let's start making function in Result() return type
| private DatafusionReader current; | ||
| private String path; | ||
| private String dataFormat; | ||
| private Consumer<List<String>> onFilesAdded; |
There was a problem hiding this comment.
Should this not be initialised in the constructor always?
There was a problem hiding this comment.
Thinking of a better way to have this neatly tied to Engine.
| private String path; | ||
| private String dataFormat; | ||
| private Consumer<List<String>> onFilesAdded; | ||
| private Consumer<List<String>> onFilesRemoved; |
There was a problem hiding this comment.
What's the usecase of it?
There was a problem hiding this comment.
Was thinking to have it when Engine.close is triggered.
This way the files of closed indices would be covered
2 participants
Description
[Describe what this change achieves]
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.