You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/02/28 08:22:07 UTC

[GitHub] [arrow] wqc200 opened a new pull request #9600: ARROW-11822: [Rust] Support case sensitive for function

wqc200 opened a new pull request #9600:
URL: https://github.com/apache/arrow/pull/9600


   SELECT database() and SELECT DATABASE () are the same and can be queried normally


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-822358732


   The Apache Arrow Rust community is moving the Rust implementation into its own dedicated github repositories [arrow-rs](https://github.com/apache/arrow-rs) and [arrow-datafusion](https://github.com/apache/arrow-datafusion). It is likely we will not merge this PR into this repository
   
   Please see the [mailing-list](https://lists.apache.org/thread.html/rce7c61cb3f703367dc00984530016e3fcb828e5a8035655fbcccf862%40%3Cdev.arrow.apache.org%3E) thread for more details
   
   We expect the process to take a few days and will follow up with a migration plan for the in-flight PRs.


-- 
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 edited a comment on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 edited a comment on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-787454075


   > 
   > 
   > Thanks @wqc200 -- this is great functionality to have.
   > 
   > I would personally like to have datafusion follow postgres when it comes to case sensitivity (rather than having it be an option).
   > 
   > @andygrove / @seddonm1 / @Dandandan what do you think about making DF follow postgres's lead (and effectively convert everything to lowercase, except identifiers explicitly quoted.
   > 
   > For example `Database` --> `database` but `"Database"` --> `Database`?
   > 
   > Here is a blog post that I found that talks about posgres sensitivity
   > https://blog.xojo.com/2016/09/28/about-postgresql-case-sensitivity/
   
   @alamb 
   I'm now using DataFusion which is in the MySQL protocol. If I didn't make the case sensitive option, I would have a problem here.But we can make the default consistent with PostGRE


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r584779892



##########
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:
       I wonder if there is any reason to copy the config or if the code could return a reference (and the caller could `clone()` it if they needed to
   
   ```suggestion
   
       fn config(&self) -> &ExecutionConfig {
           self.config
       }
   ```




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r588872840



##########
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:
       FWIW here is what postgres does for the same query (uses the lowercase, canonical function name)
   
   ```
   alamb=# select mD5('a');
                  md5                
   ----------------------------------
    0cc175b9c0f1b6a831c399e269772661
   (1 row)
   ```




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] andygrove commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-787461922


   I think it is good that we default to Postgres dialect and case-sensitivity but I would like to make it possible for users to choose other dialects and case-sensitivity if there isn't a huge burden in doing so.
   
   I can see DataFusion being incredibly useful in projects where it is necessary to mimic another database system.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r593100188



##########
File path: rust/datafusion/src/execution/context.rs
##########
@@ -495,13 +495,30 @@ impl QueryPlanner for DefaultQueryPlanner {
     }
 }
 
+/// The style of case display
+#[derive(Clone, PartialEq)]
+pub enum CaseStyle {
+    /// Case style like MySQL, SELECT mD5("a") will output SELECT mD5("a")
+    LikeMySQL,
+    /// Case style like PostgreSQL, SELECT mD5("a") will output SELECT md5("a")
+    LikePostgreSQL,

Review comment:
       ```suggestion
       /// function names in the output are displayed as they were provided:
       /// 'mD5("a")' will appear in the results as `mD5("a")`. This 
       /// mimics MySQL behavior
       PreserveCase,
       /// function names in the output are displayed in lower case:
       /// `mD5("a")` will appear in the results as  `md5("a")`This 
       /// mimics PostgreSQL behavior
       LikePostgreSQL,
   ```
   
   I think naming the enum variants based on what they do may be clearer

##########
File path: rust/datafusion/src/execution/context.rs
##########
@@ -495,13 +495,30 @@ impl QueryPlanner for DefaultQueryPlanner {
     }
 }
 
+/// The style of case display

Review comment:
       ```suggestion
   /// The style of displaying function and aggregate names in the query results
   ```

##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -963,7 +983,12 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                             .collect::<Result<Vec<Expr>>>()?
                     };
 
+                    if let CaseStyle::LikePostgreSQL = case_style {
+                        input_name = input_name.to_uppercase();

Review comment:
       I think postgres does `to_lowercase` -- this also seems to be inconsistent with the clause above (that also uses `to_lowercase`. 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r593738325



##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -963,7 +983,12 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                             .collect::<Result<Vec<Expr>>>()?
                     };
 
+                    if let CaseStyle::LikePostgreSQL = case_style {
+                        input_name = input_name.to_uppercase();

Review comment:
       @alamb 
   
   I would like to put the modification of the test case in another PR, the current PR just like that, is that OK?




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-827691826


   Hi @wqc200 I am sorry this has been a frustrating experience for you. The reason I have not merged this PR is because it does not have tests for the new features that were added
   
   Also, we implemented the case insensitive comparison this PR describes in https://github.com/apache/arrow/pull/9827
   
   I suggest:
   1. Review the current version of datafusion (which includes the case comparison  https://github.com/apache/arrow/pull/9827) and see if it works for you
   2. Pull out the change that allows the user to control the output case into a separate PR (against the new repo) with tests showing how it works
   
   


-- 
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r585519845



##########
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:
       Oh, this has nothing to do with the current PR, I'm using this function in my project, so I added pub




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #9600: ARROW-11822: [Rust] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-787415485


   https://issues.apache.org/jira/browse/ARROW-11822


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r593738121



##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -963,7 +983,12 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                             .collect::<Result<Vec<Expr>>>()?
                     };
 
+                    if let CaseStyle::LikePostgreSQL = case_style {
+                        input_name = input_name.to_uppercase();

Review comment:
       If we used to_LowerCase, many of the test cases would fail, such as the test case below:
   
   #[tokio::test]
       async fn count_basic() -> Result<()> {
           let results = execute("SELECT COUNT(c1), COUNT(c2) FROM test", 1).await?;
           assert_eq!(results.len(), 1);
   
           let expected = vec![
               "+-----------+-----------+",
               "| COUNT(c1) | COUNT(c2) |",
               "+-----------+-----------+",
               "| 10        | 10        |",
               "+-----------+-----------+",
           ];
           assert_batches_sorted_eq!(expected, &results);
           Ok(())
       }




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r591789672



##########
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:
       Yes, i fix it, use the lowercase in postgress. I add a enum with two feild, one is LikeMySQL, another is LikePostgreSQL.
   
   > select mD5('a'); 
   >          md5
   




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r593738121



##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -963,7 +983,12 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                             .collect::<Result<Vec<Expr>>>()?
                     };
 
+                    if let CaseStyle::LikePostgreSQL = case_style {
+                        input_name = input_name.to_uppercase();

Review comment:
       If we used to_LowerCase, many of the test cases would fail, such as the test case below:
   
   #[tokio::test]
       async fn count_basic() -> Result<()> {
           let results = execute("SELECT COUNT(c1), COUNT(c2) FROM test", 1).await?;
           assert_eq!(results.len(), 1);
   
           let expected = vec![
               "+-----------+-----------+",
               "| COUNT(c1) | COUNT(c2) |",
               "+-----------+-----------+",
               "| 10        | 10        |",
               "+-----------+-----------+",
           ];
           assert_batches_sorted_eq!(expected, &results);
           Ok(())
       }




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] Dandandan commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-787450447


   This would be great to have indeed  (and start doing some more testing on, I think ATM we are both case sensitive and case insensitive.
   
   I agree it would be best to start having it default to using PostgreSQL case insensitivity without it being an option, to avoid making the code more complex than needed.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] codecov-io commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-798427836


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=h1) Report
   > Merging [#9600](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=desc) (2bc66d6) into [master](https://codecov.io/gh/apache/arrow/commit/6a1320604a2021711e637c2ca586b374a593d66e?el=desc) (6a13206) will **decrease** coverage by `0.03%`.
   > The diff coverage is `56.86%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/9600/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9600      +/-   ##
   ==========================================
   - Coverage   82.57%   82.53%   -0.04%     
   ==========================================
     Files         248      248              
     Lines       58279    58357      +78     
   ==========================================
   + Hits        48121    48163      +42     
   - Misses      10158    10194      +36     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/datafusion/src/optimizer/utils.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9vcHRpbWl6ZXIvdXRpbHMucnM=) | `46.82% <0.00%> (-1.94%)` | :arrow_down: |
   | [rust/datafusion/src/physical\_plan/udaf.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3VkYWYucnM=) | `73.17% <0.00%> (-1.83%)` | :arrow_down: |
   | [rust/datafusion/src/sql/utils.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9zcWwvdXRpbHMucnM=) | `47.33% <11.76%> (-2.99%)` | :arrow_down: |
   | [rust/datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9leGVjdXRpb24vY29udGV4dC5ycw==) | `90.19% <25.00%> (-0.65%)` | :arrow_down: |
   | [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `82.96% <81.25%> (-0.43%)` | :arrow_down: |
   | [rust/datafusion/src/sql/planner.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9zcWwvcGxhbm5lci5ycw==) | `83.37% <92.59%> (+0.15%)` | :arrow_up: |
   | [rust/datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3BsYW5uZXIucnM=) | `79.62% <100.00%> (ø)` | |
   | [rust/datafusion/src/physical\_plan/udf.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3VkZi5ycw==) | `79.16% <100.00%> (+0.90%)` | :arrow_up: |
   | [rust/parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9lbmNvZGluZ3MvZW5jb2RpbmcucnM=) | `95.05% <0.00%> (+0.19%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=footer). Last update [6a13206...2bc66d6](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-787453495


   > 
   > 
   > This would be great to have indeed (and start doing some more testing on, I think ATM we are both case sensitive and case insensitive.
   > 
   > I agree it would be best to start having it default to using PostgreSQL case insensitivity without it being an option, to avoid making the code more complex than needed.
   
   Thank you very much!I'm going to do this in two steps, step one: support case-sensitive functionality, step two: make it case-insensitive by default, and fix the test cases


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-808916226


   I spent some time working on case sensitive functions this morning and have a proposed PR: https://github.com/apache/arrow/pull/9827. I think the exercise of writing tests to demonstrate the behavior was a good one.
   
   After https://github.com/apache/arrow/pull/9827 is sorted out, I plan to give similar treatment to the part of this PR that defies how identifiers are shown in the query output


-- 
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-792376112


   @alamb 
   OK, I think we should add support for different dialects


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-787454075


   > 
   > 
   > Thanks @wqc200 -- this is great functionality to have.
   > 
   > I would personally like to have datafusion follow postgres when it comes to case sensitivity (rather than having it be an option).
   > 
   > @andygrove / @seddonm1 / @Dandandan what do you think about making DF follow postgres's lead (and effectively convert everything to lowercase, except identifiers explicitly quoted.
   > 
   > For example `Database` --> `database` but `"Database"` --> `Database`?
   > 
   > Here is a blog post that I found that talks about posgres sensitivity
   > https://blog.xojo.com/2016/09/28/about-postgresql-case-sensitivity/
   
   
   I'm now using DataFusion which is in the MySQL protocol. If I didn't make the case sensitive option, I would have a problem 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-816588715


   @wqc200  do you still want / need the functionality in this PR to preserve the original case of function calls? (aka show `DATABASE` in the output rather than`database`)? The code for the case insensitive identifier comparison part was added in https://github.com/apache/arrow/pull/9827. 
   
   If so, I will find some time to get a PR in shape


-- 
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r593747416



##########
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:
       I tried to change to this way of writing, I failed to debug.So leave that for another day




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r585522814



##########
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:
       This is mainly used to display the name of the function entered by the user. 
   md5 (Utf8("a")) instead of mD5(Utf8("a")) if there is no input_name.
   
   For example:
   
   > mysql> select mD5('a');
   > +----------------------------------+
   > | mD5(Utf8("a"))                   |
   > +----------------------------------+
   > | 0cc175b9c0f1b6a831c399e269772661 |
   > +----------------------------------+
   > 1 row in set (0.01 sec)
   




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-795921385


   @alamb 
   
   I added an ENUM, which now supports PostgreSQL, so I think this PR can be merged first. If there are any problems, we can raise PR for upgrading later.
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-805890578


   @alamb 
   @Dandandan 
   
   This PR has been more than 1 month, when to merge?


-- 
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-791751613


   I have some thoughts on this PR -- I plan to write them up coherently tomorrow


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] codecov-io edited a comment on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-798427836


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=h1) Report
   > Merging [#9600](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=desc) (c8608dd) into [master](https://codecov.io/gh/apache/arrow/commit/2b87dfc5da67dc39d18ccc1d49ed5212b468496e?el=desc) (2b87dfc) will **decrease** coverage by `0.03%`.
   > The diff coverage is `56.43%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/9600/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9600      +/-   ##
   ==========================================
   - Coverage   82.68%   82.64%   -0.04%     
   ==========================================
     Files         256      256              
     Lines       60105    60183      +78     
   ==========================================
   + Hits        49698    49740      +42     
   - Misses      10407    10443      +36     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/datafusion/src/optimizer/utils.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9vcHRpbWl6ZXIvdXRpbHMucnM=) | `47.65% <0.00%> (-1.94%)` | :arrow_down: |
   | [rust/datafusion/src/physical\_plan/udaf.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3VkYWYucnM=) | `73.17% <0.00%> (-1.83%)` | :arrow_down: |
   | [rust/datafusion/src/sql/utils.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9zcWwvdXRpbHMucnM=) | `47.33% <11.76%> (-2.99%)` | :arrow_down: |
   | [rust/datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9leGVjdXRpb24vY29udGV4dC5ycw==) | `91.66% <25.00%> (-0.52%)` | :arrow_down: |
   | [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `82.96% <81.25%> (-0.43%)` | :arrow_down: |
   | [rust/datafusion/src/sql/planner.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9zcWwvcGxhbm5lci5ycw==) | `83.40% <92.30%> (+0.14%)` | :arrow_up: |
   | [rust/datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3BsYW5uZXIucnM=) | `79.77% <100.00%> (ø)` | |
   | [rust/datafusion/src/physical\_plan/udf.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3VkZi5ycw==) | `79.16% <100.00%> (+0.90%)` | :arrow_up: |
   | [rust/parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow/pull/9600/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9lbmNvZGluZ3MvZW5jb2RpbmcucnM=) | `95.05% <0.00%> (+0.19%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=footer). Last update [2b87dfc...c8608dd](https://codecov.io/gh/apache/arrow/pull/9600?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


-- 
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-787524146


   > Thank you very much!I'm going to do this in two steps, step one: support case-sensitive functionality, step two: make it case-insensitive by default, and fix the test cases
   
   
   Makes sense @wqc200  -- I will try and review this PR carefully tomorrow. 


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r593738325



##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -963,7 +983,12 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                             .collect::<Result<Vec<Expr>>>()?
                     };
 
+                    if let CaseStyle::LikePostgreSQL = case_style {
+                        input_name = input_name.to_uppercase();

Review comment:
       @alamb 
   
   I would like to put the modification of the test case in another PR, the current PR just like that, is that OK?




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-803288758


   > 
   > 
   > Thank you very much @wqc200 -- I think this PR's code is now ready to go except for are some basic tests that show the difference in behavior when setting `CaseStyle` and `cast_sensitive` (not changing the defaults).
   > 
   > The purpose of such tests are to ensure no one breaks this feature accidentally when making changes elsewhere and in the future.
   > 
   > I am willing to write these tests this PR can be merged given how much time you have spent on this already -- let me know if you would like help to do so
   
   Ok


-- 
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 edited a comment on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 edited a comment on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-787454075


   > 
   > 
   > Thanks @wqc200 -- this is great functionality to have.
   > 
   > I would personally like to have datafusion follow postgres when it comes to case sensitivity (rather than having it be an option).
   > 
   > @andygrove / @seddonm1 / @Dandandan what do you think about making DF follow postgres's lead (and effectively convert everything to lowercase, except identifiers explicitly quoted.
   > 
   > For example `Database` --> `database` but `"Database"` --> `Database`?
   > 
   > Here is a blog post that I found that talks about posgres sensitivity
   > https://blog.xojo.com/2016/09/28/about-postgresql-case-sensitivity/
   
   @alamb 
   I'm now using DataFusion which is in the MySQL protocol. If I didn't make the case sensitive option, I would have a problem 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb closed pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb closed pull request #9600:
URL: https://github.com/apache/arrow/pull/9600


   


-- 
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-827687060


   @andygrove @alamb 
   
   You're making me go back and forth. I've been working on this PR for months, but you just won't merge it.Now you have built a new warehouse, and I need to do this PR again. May I ask what you really want to do?


-- 
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-805940510


   @wqc200  -- from my perspective all this PR is waiting for is tests:
   
   > some basic tests that show the difference in behavior when setting CaseStyle and cast_sensitive (not changing the defaults).
   
   I do have writing such tests on my personal work list but I have not yet had a chance to write them
   


-- 
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:
users@infra.apache.org

[GitHub] [arrow] wqc200 commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
wqc200 commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r585517702



##########
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:
       Yes, you are right




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-831243330


   https://github.com/apache/arrow/pull/10096 has removed the arrow implementation from this repository (it now resides in https://github.com/apache/arrow-rs and https://github.com/apache/arrow-datafusion) in the hopes of streamlining the development process
   
   Please re-target this PR (let us know if you need help doing so) to one/both of the new repositories. 
   
   Thank you for understanding and helping to make arrow-rs and datafusion better


-- 
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:
users@infra.apache.org

[GitHub] [arrow] Dandandan commented on a change in pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#discussion_r593739227



##########
File path: rust/datafusion/src/execution/context.rs
##########
@@ -495,13 +495,30 @@ impl QueryPlanner for DefaultQueryPlanner {
     }
 }
 
+/// The style of case display
+#[derive(Clone, PartialEq)]
+pub enum CaseStyle {
+    /// Case style like MySQL, SELECT mD5("a") will output SELECT mD5("a")
+    LikeMySQL,
+    /// Case style like PostgreSQL, SELECT mD5("a") will output SELECT md5("a")
+    LikePostgreSQL,
+}
+
 /// Configuration options for execution context
 #[derive(Clone)]
 pub struct ExecutionConfig {
     /// Number of concurrent threads for query execution.
     pub concurrency: usize,
     /// Default batch size when reading data sources
     pub batch_size: usize,
+    /// Will function names be searched using case-sensitive matching.

Review comment:
       ```suggestion
       /// Whether to use case-sensitive matching for function names.
   ```




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-798885671


   > The above is the primary reason why I did not introduce this idea before; it will lead to either no one changing that parameter once the system is running
   
   Yes -- this is what I think will happen. I view this configuration parameter as something systems that are built on top of DataFusion will configure statically based on their needs and not change on a per-query basis (even though you could given the way the code is written)
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [arrow] jorgecarleitao commented on pull request #9600: ARROW-11822: [Rust][Datafusion] Support case sensitive for function

Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #9600:
URL: https://github.com/apache/arrow/pull/9600#issuecomment-798374010


   How an SQL statement is converted to a logical plan includes two aspects:
   
   1. what is the logical plan
   2. what is the output schema
   
   IMO the principle of least surprise is that the column name becomes what the user wrote on the SQL.
   
   A schema is typically part of the contract between the consumer and producer, and there are non-trivial implications of supporting two namings as a configuration: the resulting schema of a query changes depending on the configuration of the engine. This means that if a user writes the statement `SELECT SQRT(c1) FROM T` and runs on a system configured as case-insensitive, the resulting schema is `sqrt(c1)`, but if the user runs it on a case-sensitive system, the resulting schema is `SQRT(c1)`.
   
   The above is the primary reason why I did not introduce this idea before; it will lead to either no one changing that parameter once the system is running, or a constant fight in trying to have the systems always configured in the same way to avoid schema changes derived from a change in the configuration.
   
   With that said, as long as we are consistent and document the tradeoffs somewhere, I do not have a strong option in either direction (i.e. allow case-sensitive or only support one).


----------------------------------------------------------------
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:
users@infra.apache.org