feat: Enable Disk Caching for Multiple Sources (Supersedes PR#30) #148
Conversation
…e configurations Signed-off-by: shamb0 <[email protected]>
| @@ -51,7 +51,7 @@ impl OptionValidator for JsonOption { | |||
| } | |||
| } | |||
|
|
|||
| pub fn create_view( | |||
| pub fn create_duckdb_relation( | |||
There was a problem hiding this comment.
Why are you renaming these functions? This again is making the diff much larger and harder to review. You're not even changing the code of the function, so why does it need a new name?
| pub fn create_duckdb_relation( | ||
| table_name: &str, | ||
| schema_name: &str, | ||
| table_options: HashMap<String, String>, | ||
| handler: FdwHandler, | ||
| ) -> Result<()> { | ||
| connection::execute( | ||
| format!("CREATE SCHEMA IF NOT EXISTS {schema_name}").as_str(), | ||
| [], | ||
| )?; | ||
|
|
||
| match handler { | ||
| FdwHandler::Csv => { | ||
| connection::create_csv_relation(table_name, schema_name, table_options)?; | ||
| } | ||
| FdwHandler::Delta => { | ||
| connection::create_delta_relation(table_name, schema_name, table_options)?; | ||
| } | ||
| FdwHandler::Iceberg => { | ||
| connection::create_iceberg_relation(table_name, schema_name, table_options)?; | ||
| } | ||
| FdwHandler::Parquet => { | ||
| connection::create_parquet_relation(table_name, schema_name, table_options)?; | ||
| } | ||
| FdwHandler::Spatial => { | ||
| connection::create_spatial_relation(table_name, schema_name, table_options)?; | ||
| } | ||
| FdwHandler::Json => { | ||
| connection::create_json_relation(table_name, schema_name, table_options)?; | ||
| } | ||
| _ => { | ||
| bail!("got unexpected fdw_handler") | ||
| } | ||
| }; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
If you think this is a better way to handle creating relations, could you please pull this out into a separate PR and submit it independently? This seems to me irrelevant to the feature at stakes here, which is making things harder to review
| pub fn register_duckdb_view( | ||
| table_name: &str, | ||
| schema_name: &str, | ||
| table_options: HashMap<String, String>, | ||
| user_mapping_options: HashMap<String, String>, | ||
| handler: FdwHandler, | ||
| ) -> Result<()> { | ||
| if !user_mapping_options.is_empty() { | ||
| connection::create_secret(DEFAULT_SECRET, user_mapping_options)?; | ||
| } | ||
|
|
||
| if !connection::view_exists(table_name, schema_name)? { | ||
| // Initialize DuckDB view | ||
| connection::execute( | ||
| format!("CREATE SCHEMA IF NOT EXISTS {schema_name}").as_str(), | ||
| [], | ||
| )?; | ||
|
|
||
| match handler { | ||
| FdwHandler::Csv => { | ||
| connection::create_csv_view(table_name, schema_name, table_options)?; | ||
| } | ||
| FdwHandler::Delta => { | ||
| connection::create_delta_view(table_name, schema_name, table_options)?; | ||
| } | ||
| FdwHandler::Iceberg => { | ||
| connection::create_iceberg_view(table_name, schema_name, table_options)?; | ||
| } | ||
| FdwHandler::Parquet => { | ||
| connection::create_parquet_view(table_name, schema_name, table_options)?; | ||
| } | ||
| FdwHandler::Spatial => { | ||
| connection::create_spatial_view(table_name, schema_name, table_options)?; | ||
| } | ||
| FdwHandler::Json => { | ||
| connection::create_json_view(table_name, schema_name, table_options)?; | ||
| } | ||
| _ => { | ||
| bail!("got unexpected fdw_handler") | ||
| } | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Why move this? It doesn't seem related to your new feature
There was a problem hiding this comment.
Lots of code being moved around but seemingly not changed. Could you please share the rational why?
If you need to restructure things to enable your feature, which I can't quite tell at first glance, please submit a PR that this depends on. The diff on this PR is 800 lines yet I suspect you're only adding a much smaller fraction of new lines of code.
Please rescope this PR so it's reviewable.
|
Hi @philippemnoel, Thank you for the review comments, I really appreciate it. Moving forward, I'll aim for smaller PRs with minimal changes to make them easier to review. |
This PR is part of a pair; please review, validate, and consider merging both.
paradedb/paradedb#1751
#148
Scope of this PR:
This PR supersedes the following previous submissions:
It is based on consolidated requirements and feedback from the review comments on the above closed PRs.
What
pg_analytics.paradedb/cargo-paradedb/src/pga_benches/pga_benchlogs_hsp_pq.rs.Why
How