-
Notifications
You must be signed in to change notification settings - Fork 2k
Closed
Labels
bugSomething isn't workingSomething isn't working
Description
Describe the bug
If you execute SHOW NONSENSE via FlightSQL (or any other value after SHOW) you get an empty result:
To Reproduce
DataFusion CLI v40.0.0
> show nonsense;
+------+-------+
| name | value |
+------+-------+
+------+-------+
0 row(s) fetched.
Elapsed 0.054 seconds.Expected behavior
We expect the query to error/fail
However, other values like SHOW ALL should continue to work, for example
DataFusion CLI v40.0.0
> show all;
+-------------------------------------------------------------------------+---------------------------+
| name | value |
+-------------------------------------------------------------------------+---------------------------+
| datafusion.catalog.create_default_catalog_and_schema | true |
| datafusion.catalog.default_catalog | datafusion |
| datafusion.catalog.default_schema | public |
| datafusion.catalog.format | |
| datafusion.catalog.has_header | false |
| datafusion.catalog.information_schema | true |
| datafusion.catalog.location | |
| datafusion.execution.aggregate.scalar_update_factor | 10 |
| datafusion.execution.batch_size | 8192 |
| datafusion.execution.coalesce_batches | true |
| datafusion.execution.collect_statistics | false |
| datafusion.execution.enable_recursive_ctes | true |
And
> show datafusion.catalog.format ;
+---------------------------+-------+
| name | value |
+---------------------------+-------+
| datafusion.catalog.format | |
+---------------------------+-------+
1 row(s) fetched.
Elapsed 0.006 seconds.Additional context
This is based on a code / report filed by @crepererum and @itsjunetime
/// `{variable}` is not a valid config value and not a special-cased value, like `all`. If this
/// statement does contain such a statement, return an error.
fn check_stmt_for_invalid_show(state: &SessionState, stmt: &Statement) -> Result<()> {
// these are the special cases which, if added after a `SHOW` command
static SHOW_SPECIAL_CASES: [&str; 3] = ["all", "timezone", "time.zone"];
match stmt {
Statement::Statement(query) => match **query {
SQLStatement::ShowVariable { variable: ref vars } => {
// if they specified `verbose` as the last argument, that's also allowed and
// special-cased. Once again, pulled from `show_variable_to_plan()`.
let vars = if vars.last().is_some_and(|id| id.value == "verbose") {
&vars[..vars.len() - 1]
} else {
vars.as_slice()
};
// if someone inputs something like datafusion.configuration.something, `vars`
// is `["datafusion", "configuration", "something"]`. For some reason, it's
// split out (it seems that you could also enter it like `SHOW datafusion
// contiruation something` and that would be valid as well, based on
// `show_variable_to_plan()`) and we need to join it back together to get the
// whole variable that we want to check against the valid options
let idents_str = vars
.iter()
.map(|id| id.to_string())
.collect::<Vec<_>>()
.join(".");
// try to find the first variable that doesn't exist and isn't special-cased
// for convenience
let is_invalid = !state
// config_options is used to create the information_schema table, so that's
// why we're using it here to check against the requested variables.
.config_options()
.entries()
.iter()
.map(|opt| opt.key.as_str())
.chain(SHOW_SPECIAL_CASES)
.any(|var| var == idents_str);
if is_invalid {
Err(plan_datafusion_err!("Cannot show variable '{idents_str}' as it is not a valid configuration value"))
} else {
Ok(())
}
}
_ => Ok(()),
},
_ => Ok(()),
}
}Here are some error cases (should be in slt in datafusion tests)
assert_eq!(
planner.sql("SHOW NONSENSE", StatementParams::default()).await.unwrap_err().strip_backtrace(),
"Error during planning: Cannot show variable 'NONSENSE' as it is not a valid configuration value"
);
assert_eq!(
planner.sql("SHOW whatever", StatementParams::default()).await.unwrap_err().strip_backtrace(),
"Error during planning: Cannot show variable 'whatever' as it is not a valid configuration value"
);
assert_eq!(
planner.sql("SHOW still invalid verbose", StatementParams::default()).await.unwrap_err().strip_backtrace(),
"Error during planning: Cannot show variable 'still.invalid' as it is not a valid configuration value"
);
// message is kind of confusing. Should we special-case this error case too?
assert_eq!(
planner.sql("SHOW verbose", StatementParams::default()).await.unwrap_err().strip_backtrace(),
"Error during planning: Cannot show variable '' as it is not a valid configuration value"
);
}Here are some postitive cases (may already be covered by slt)
assert!(planner
.sql("SHOW all", StatementParams::default())
.await
.is_ok());
assert!(planner
.sql("SHOW all verbose", StatementParams::default())
.await
.is_ok());
assert!(planner
.sql(
"SHOW datafusion.catalog.location",Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
bugSomething isn't workingSomething isn't working