martin-g commented on code in PR #25887:
URL: https://github.com/apache/datafusion/pull/25887#discussion_r4367052601
##########
datafusion/sql/src/statement.rs:
##########
@@ -3135,11 +3137,12 @@ FROM (
&self,
sql_table_name: ObjectName,
) -> Result<LogicalPlan> {
- if !self.has_table("information_schema", "tables") {
+ let Some(tables_table_ref) = self.resolve_info_table("views") else {
Review Comment:
nit:
```suggestion
let Some(views_table_ref) = self.resolve_info_table("views") else {
```
to be clearer what it is about
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -81,10 +82,55 @@ pub struct InformationSchemaProvider {
}
impl InformationSchemaProvider {
+ /// Creates a new [`InformationSchemaProvider`] for the provided
`catalog_list`
Review Comment:
```suggestion
/// Creates a new [`InformationSchemaProvider`] for the provided
`catalog`
```
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -150,6 +202,17 @@ impl InformationSchemaConfig {
}
}
+ if let Some(system_catalog_name) = &self.system_catalog_name {
+ for table_name in INFORMATION_SCHEMA_TABLES {
+ builder.add_table(
Review Comment:
according to
https://github.com/apache/datafusion/pull/25887/changes#diff-8303a17e3c522b51c73c7fb0474b775746730873fc953e0c52c3fb7e67e84d5bR319
the system catalog could be a normal catalog this will add the same tables
second time, no ?
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -87,13 +89,16 @@ use datafusion_sql::{
use async_trait::async_trait;
use chrono::{DateTime, Utc};
+use datafusion_sql::parser::ExplainStatement;
Review Comment:
line 86 imports `Statement` conditionally: `#[cfg(feature = "sql")]`
does this import need the same ?
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -645,6 +711,24 @@ impl SessionState {
query.statement_to_plan(statement)
}
+ #[cfg(feature = "sql")]
+ fn requires_information_schema(statement: &Statement) -> bool {
+ match &statement {
+ Statement::Explain(ExplainStatement { statement, .. }) => {
+ Self::requires_information_schema(statement)
+ }
+ Statement::Statement(s) => matches!(
+ s.as_ref(),
+ SQLStatement::ShowCreate { .. }
+ | SQLStatement::ShowTables { .. }
+ | SQLStatement::ShowColumns { .. }
+ | SQLStatement::ShowVariable { .. }
+ | SQLStatement::ShowFunctions { .. }
Review Comment:
ShowStatus, ShowVariables and ShowCollation are no more matched. Intentional
?
##########
datafusion/sql/src/statement.rs:
##########
@@ -1675,14 +1676,16 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
/// Generate a logical plan from a "SHOW TABLES" query
fn show_tables_to_plan(&self) -> Result<LogicalPlan> {
- if self.has_table("information_schema", "tables") {
- let query = "SELECT * FROM information_schema.tables;";
- let mut rewrite = DFParser::parse_sql(query)?;
- assert_eq!(rewrite.len(), 1);
- self.statement_to_plan(rewrite.pop_front().unwrap()) // length of
rewrite is 1
- } else {
- plan_err!("SHOW TABLES is not supported unless information_schema
is enabled")
- }
+ let Some(tables_table_ref) = self.resolve_info_table("tables") else {
+ return plan_err!(
+ "SHOW TABLES is not supported unless information_schema is
enabled"
+ );
+ };
+
+ let query = format!("SELECT * FROM {tables_table_ref};");
Review Comment:
```suggestion
let query = format!("SELECT * FROM {};",
tables_table_ref.to_quoted_string());
```
quote that table name to support system catalog names like `System`
##########
datafusion/sql/src/statement.rs:
##########
Review Comment:
resolve_info_table("routines") ?!
##########
datafusion/sql/src/statement.rs:
##########
Review Comment:
shouldn't this use `resolve_info_table("parameters")` to resolve the correct
catalog+schema+table ?
##########
datafusion/sql/src/statement.rs:
##########
Review Comment:
same here
--
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]
