alamb commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r584777400
##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -927,7 +931,15 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
}
SQLExpr::Function(function) => {
- let name: String = function.name.to_string();
+ let name;
+ let input_name = function.name.to_string();
+
+ let case_sensitive =
self.schema_provider.get_config().case_sensitive;
+ if !case_sensitive {
+ name = input_name.to_lowercase();
+ } else {
+ name = input_name.clone();
+ }
Review comment:
I think the following formulation would be more idiomatic rust (and
save a copy of `input_name`).
```suggestion
let input_name = function.name.to_string();
let case_sensitive =
self.schema_provider.get_config().case_sensitive;
let name = if !case_sensitive {
input_name.to_lowercase();
} else {
input_name;
}
```
##########
File path: rust/datafusion/src/execution/context.rs
##########
@@ -502,6 +502,8 @@ pub struct ExecutionConfig {
pub concurrency: usize,
/// Default batch size when reading data sources
pub batch_size: usize,
+ /// Case sensitive
Review comment:
```suggestion
/// Will function names be searched using case-sensitive matching.
/// If `false` both `"SELECT COUNT(*) FROM t;` and "`SELECT count(*)
FROM t;`
/// can be used to compute the `COUNT` aggregate. If `true` then only
/// `"SELECT count(*) FROM t"` can be used.
/// Defaults to `true`
```
##########
File path: rust/arrow-flight/src/arrow.flight.protocol.rs
##########
@@ -499,8 +499,9 @@ pub mod flight_service_server {
#[async_trait]
pub trait FlightService: Send + Sync + 'static {
#[doc = "Server streaming response type for the Handshake method."]
- type HandshakeStream: Stream<Item = Result<super::HandshakeResponse,
tonic::Status>>
Review comment:
I don't think the changes in this file are needed, are they?
##########
File path: rust/datafusion/src/execution/context.rs
##########
@@ -589,6 +598,10 @@ impl ContextProvider for ExecutionContextState {
fn get_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>> {
self.aggregate_functions.get(name).cloned()
}
+
+ fn get_config(&self) -> ExecutionConfig {
+ self.config.clone()
+ }
Review comment:
```suggestion
fn config(&self) -> &ExecutionConfig {
self.config
}
```
##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -343,7 +346,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
}
/// Generate a logic plan from an SQL select
- fn select_to_plan(&self, select: &Select) -> Result<LogicalPlan> {
+ pub fn select_to_plan(&self, select: &Select) -> Result<LogicalPlan> {
Review comment:
Does this need to be made `pub`?
##########
File path: rust/datafusion/src/logical_plan/expr.rs
##########
@@ -160,20 +160,26 @@ pub enum Expr {
},
/// Represents the call of a built-in scalar function with a set of
arguments.
ScalarFunction {
+ /// The input name of the function
+ input_name: String,
Review comment:
Also, I looked through the diff and I couldn't figure out what
`input_name` is actually used for. Is this new field needed?
##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -927,7 +931,15 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
}
SQLExpr::Function(function) => {
- let name: String = function.name.to_string();
+ let name;
+ let input_name = function.name.to_string();
+
+ let case_sensitive =
self.schema_provider.get_config().case_sensitive;
+ if !case_sensitive {
+ name = input_name.to_lowercase();
+ } else {
+ name = input_name.clone();
+ }
Review comment:
This logic seems to implement case *IN*sensitive logic. Namely if
`case_sensitive` is true, I would expect `count` != `COUNT` but this PR does
the opposite.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
