Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register SQL planners in SessionState::new() #11216

Closed
Tracked by #11207
jayzhan211 opened this issue Jul 2, 2024 · 5 comments · Fixed by #11253
Closed
Tracked by #11207

Register SQL planners in SessionState::new() #11216

jayzhan211 opened this issue Jul 2, 2024 · 5 comments · Fixed by #11253
Assignees

Comments

@jayzhan211
Copy link
Contributor

          It might make sense to move the registering of these planners into the constructor of SessionState rather than here (so users could potentially more carefully control the planner content). But we can do that as a follow on PR

Originally posted by @alamb in #11208 (comment)

@dharanad
Copy link
Contributor

dharanad commented Jul 2, 2024

hello @jayzhan211 , i would like to work on this. QQ: Is this issue any straight as it sounds or there is more to it?

@jayzhan211
Copy link
Contributor Author

hello @jayzhan211 , i would like to work on this. QQ: Is this issue any straight as it sounds or there is more to it?

A little more than it. Ideally we need to update map capacity in provider with the number of references. But, I think you can try it.

let references = self.resolve_table_references(&statement)?;
let mut provider = SessionContextProvider {
state: self,
tables: HashMap::with_capacity(references.len()),
};
for reference in references {
let resolved = &self.resolve_table_ref(reference);
if let Entry::Vacant(v) = provider.tables.entry(resolved.to_string()) {
if let Ok(schema) = self.schema_for_ref(resolved.clone()) {
if let Some(table) = schema.table(&resolved.table).await? {
v.insert(provider_as_source(table));
}
}
}
}
let query = self.build_sql_query_planner(&provider);

@dharanad
Copy link
Contributor

dharanad commented Jul 2, 2024

take

@dharanad
Copy link
Contributor

dharanad commented Jul 3, 2024

hello @jayzhan211 , i would like to work on this. QQ: Is this issue any straight as it sounds or there is more to it?

A little more than it. Ideally we need to update map capacity in provider with the number of references. But, I think you can try it.

let references = self.resolve_table_references(&statement)?;
let mut provider = SessionContextProvider {
state: self,
tables: HashMap::with_capacity(references.len()),
};
for reference in references {
let resolved = &self.resolve_table_ref(reference);
if let Entry::Vacant(v) = provider.tables.entry(resolved.to_string()) {
if let Ok(schema) = self.schema_for_ref(resolved.clone()) {
if let Some(table) = schema.table(&resolved.table).await? {
v.insert(provider_as_source(table));
}
}
}
}
let query = self.build_sql_query_planner(&provider);

Do you mean something like this

        let mut provider = SessionContextProvider {
            state: self,
            tables: HashMap::with_capacity(references.len() + self.user_defined_sql_planners.len()),
        };

If yes, why is required ? I this that we are already updating the capacity with reference count. If i misinterpreted it can you please elaborate more ?

@alamb
Copy link
Contributor

alamb commented Jul 5, 2024

Update here is I think the current formulation is about as optimal as we can get it:

let user_defined_sql_planners: Vec<Arc<dyn UserDefinedSQLPlanner>> = vec![
Arc::new(functions::core::planner::CoreFunctionPlanner::default()),
// register crate of array expressions (if enabled)
#[cfg(feature = "array_expressions")]
Arc::new(functions_array::planner::ArrayFunctionPlanner),
#[cfg(feature = "array_expressions")]
Arc::new(functions_array::planner::FieldAccessPlanner),
#[cfg(feature = "datetime_expressions")]
Arc::new(functions::datetime::planner::ExtractPlanner),

(namely to make a Vec with a size known at compile time

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants