ARROW-11822: [Rust][Datafusion] Support case sensitive for function by wqc200 · Pull Request #9600 · apache/arrow

wqc200 · 2021-02-28T08:21:58Z

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

ARROW-11778: [Rust] Cast from LargeUtf8 to Numerical and temporal types

github-actions · 2021-02-28T08:22:16Z

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

alamb

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/

Dandandan · 2021-02-28T13:14:11Z

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.

wqc200 · 2021-02-28T13:37:55Z

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

wqc200 · 2021-02-28T13:42:15Z

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

andygrove · 2021-02-28T14:32:38Z

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.

alamb · 2021-02-28T21:13:35Z

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.

alamb

Thank you very much for the contribution @wqc200 -- this looks like a great start.

I would like to request that you add a test that changes the default value of case_sensitive and demonstrate that errors still occur.

rust/datafusion/src/sql/planner.rs

rust/datafusion/src/execution/context.rs

rust/arrow-flight/src/arrow.flight.protocol.rs

rust/datafusion/src/execution/context.rs

alamb · 2021-03-01T16:19:03Z

rust/datafusion/src/sql/planner.rs


    /// 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> {


Does this need to be made pub?

Oh, this has nothing to do with the current PR, I'm using this function in my project, so I added pub

alamb · 2021-03-01T16:21:40Z

rust/datafusion/src/logical_plan/expr.rs

    /// Represents the call of a built-in scalar function with a set of arguments.
    ScalarFunction {
+        /// The input name of the function
+        input_name: String,


Also, I looked through the diff and I couldn't figure out what input_name is actually used for. Is this new field needed?

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)

FWIW here is what postgres does for the same query (uses the lowercase, canonical function name)

alamb=# select mD5('a'); md5 ---------------------------------- 0cc175b9c0f1b6a831c399e269772661 (1 row)

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

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb

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

wqc200 · 2021-03-20T10:44:00Z

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

merge official branch

merge offical branch

wqc200 · 2021-03-24T14:57:34Z

@alamb
@Dandandan

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

alamb · 2021-03-24T15:49:47Z

@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

merge from official branch

alamb

While trying to write tests for this PR I discovered some inconsistencies:

Aggregate functions already (prior to this PR) are checked using case insensitive comparison (e.g. select MAX(x) and select max(x) both work - the code is here
scalar functions are checked using case sensitive comparison

This PR has strange behavior that the case_sensitive setting only applies to scalar functions (and aggregates still always use case insensitive comparisons).

Thus I plan to break this PR into two new PRs -- one for the CaseStyle setting and one that proposes to make function name lookup always case insensitive (and thus consistent between aggregates and scalar functions)

alamb · 2021-03-28T15:58:16Z

I spent some time working on case sensitive functions this morning and have a proposed PR: #9827. I think the exercise of writing tests to demonstrate the behavior was a good one.

After #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

merge from official branch

@wqc200

…r functions and aggregates Broken out from #9600 by @wqc200. Note this does not contain the part of #9600 that controls the output display of functions. # Rationale Aggregate functions are checked using case insensitive comparison (e.g. `select MAX(x)` and `select max(x)` both work - the code is [here](https://github.com/apache/arrow/blob/356c300c5ee1e2b23a83652514af11e3a731d596/rust/datafusion/src/physical_plan/aggregates.rs#L75) However, scalar functions, user defined aggregates, and user defined functions, are checked using case sensitive comparisons (e.g. `select sqrt(x)` works while `select SQRT` does not. Postgres always uses case insensitive comparison: ``` alamb=# select sqrt(x) from foo; sqrt ------ (0 rows) alamb=# select SQRT(x) from foo; sqrt ------ (0 rows) ``` # Changes Always use case insensitive comparisons for unquoted identifier comparison, both for consistency within DataFusion as well as consistency with Postgres (and the SQL standard) Adds tests that demonstrate the behavior # Notes This PR changes how user defined functions are resolved in SQL queries. If a user registers two functions with names `"my_sqrt"` and `"MY_SQRT"` previously they could both be called individually. After this PR `my_sqrt` will be called unless the user specifically put `"SQRT"` (in quotes) in their query. Closes #9827 from alamb/case_insensitive_functions Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

@wqc200

…r functions and aggregates Broken out from apache#9600 by @wqc200. Note this does not contain the part of apache#9600 that controls the output display of functions. # Rationale Aggregate functions are checked using case insensitive comparison (e.g. `select MAX(x)` and `select max(x)` both work - the code is [here](https://github.com/apache/arrow/blob/356c300c5ee1e2b23a83652514af11e3a731d596/rust/datafusion/src/physical_plan/aggregates.rs#L75) However, scalar functions, user defined aggregates, and user defined functions, are checked using case sensitive comparisons (e.g. `select sqrt(x)` works while `select SQRT` does not. Postgres always uses case insensitive comparison: ``` alamb=# select sqrt(x) from foo; sqrt ------ (0 rows) alamb=# select SQRT(x) from foo; sqrt ------ (0 rows) ``` # Changes Always use case insensitive comparisons for unquoted identifier comparison, both for consistency within DataFusion as well as consistency with Postgres (and the SQL standard) Adds tests that demonstrate the behavior # Notes This PR changes how user defined functions are resolved in SQL queries. If a user registers two functions with names `"my_sqrt"` and `"MY_SQRT"` previously they could both be called individually. After this PR `my_sqrt` will be called unless the user specifically put `"SQRT"` (in quotes) in their query. Closes apache#9827 from alamb/case_insensitive_functions Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2021-04-09T10:31:42Z

@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 thandatabase)? The code for the case insensitive identifier comparison part was added in #9827.

If so, I will find some time to get a PR in shape

merge from official

alamb · 2021-04-19T10:23:28Z

The Apache Arrow Rust community is moving the Rust implementation into its own dedicated github repositories arrow-rs and arrow-datafusion. It is likely we will not merge this PR into this repository

Please see the mailing-list 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.

wqc200 · 2021-04-27T15:11:28Z

@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?

alamb · 2021-04-27T15:18:03Z

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 #9827

I suggest:

Review the current version of datafusion (which includes the case comparison ARROW-11822: [Rust][Datafusion] Support case sensitive comparisons for functions and aggregates #9827) and see if it works for you
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

alamb · 2021-05-03T13:00:46Z

#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

wqc200 added 4 commits February 27, 2021 23:08

Merge pull request #9 from apache/master

e45e355

ARROW-11778: [Rust] Cast from LargeUtf8 to Numerical and temporal types

Make some functions public

fadef3f

Support case sensitive

97fb5cd

remove to_string()

122ab74

github-actions bot added Component: Rust - DataFusion Component: Rust labels Feb 28, 2021

remove unused variable fun

6793160

wqc200 changed the title ~~ARROW-11822: [Rust] Support case sensitive for function~~ ARROW-11822: [Rust][datafusion] Support case sensitive for function Feb 28, 2021

wqc200 changed the title ~~ARROW-11822: [Rust][datafusion] Support case sensitive for function~~ ARROW-11822: [Rust][Datafusion] Support case sensitive for function Feb 28, 2021

wqc200 added 4 commits February 28, 2021 16:49

equality checks against false can be replaced by a negation

33a1a9e

format code

33a1b95

format code

ea8ca2a

format code

a2e5eac

alamb reviewed Feb 28, 2021

View reviewed changes

fix test

f897970

github-actions bot added the Component: FlightRPC label Feb 28, 2021

alamb reviewed Mar 1, 2021

View reviewed changes

wqc200 and others added 7 commits March 2, 2021 20:16

Update rust/datafusion/src/sql/planner.rs

b135fe8

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update rust/datafusion/src/execution/context.rs

2092439

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update rust/datafusion/src/execution/context.rs

fb1a41d

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

remove get_config to config

474cb61

Merge branch 'master' of https://github.com/wqc200/arrow

b2e49cf

fix bug about input_name

af1568f

make config with arc

8fa5b29

alamb reviewed Mar 14, 2021

View reviewed changes

wqc200 added 2 commits March 20, 2021 18:45

Merge pull request #14 from apache/master

f09f10d

merge official branch

Merge pull request #15 from apache/master

acf2c0d

merge offical branch

wqc200 added 3 commits March 26, 2021 07:20

merge from the official branch

53d3492

Merge branch 'apache-master'

910aba4

Merge pull request #17 from apache/master

93f57de

merge from official branch

alamb reviewed Mar 28, 2021

View reviewed changes

alamb mentioned this pull request Mar 28, 2021

ARROW-11822: [Rust][Datafusion] Support case sensitive comparisons for functions and aggregates #9827

Closed

wqc200 added 3 commits March 31, 2021 23:42

merge from offical branch

4624955

Merge branch 'apache-master'

cd336f1

Merge pull request #19 from apache/master

c8608dd

merge from official branch

Merge branch 'master' into master

167e501

Merge pull request #20 from apache/master

c12c243

merge from official

alamb closed this May 3, 2021

alamb mentioned this pull request May 7, 2021

Improve column naming by aliasing with expression name apache/datafusion#280

Closed

asfimport mentioned this pull request May 3, 2021

Support case sensitive for function #27671

Closed

Conversation

wqc200 commented Feb 28, 2021

Uh oh!

github-actions bot commented Feb 28, 2021

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Dandandan commented Feb 28, 2021

Uh oh!

wqc200 commented Feb 28, 2021

Uh oh!

wqc200 commented Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andygrove commented Feb 28, 2021

Uh oh!

alamb commented Feb 28, 2021

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alamb Mar 1, 2021

Choose a reason for hiding this comment

Uh oh!

wqc200 Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

alamb Mar 1, 2021

Choose a reason for hiding this comment

Uh oh!

wqc200 Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

alamb Mar 6, 2021

Choose a reason for hiding this comment

Uh oh!

wqc200 Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

wqc200 commented Mar 20, 2021

Uh oh!

wqc200 commented Mar 24, 2021

Uh oh!

alamb commented Mar 24, 2021

Uh oh!

alamb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 28, 2021

Uh oh!

alamb commented Apr 9, 2021

Uh oh!

alamb commented Apr 19, 2021

Uh oh!

wqc200 commented Apr 27, 2021

Uh oh!

alamb commented Apr 27, 2021

Uh oh!

alamb commented May 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wqc200 commented Feb 28, 2021 •

edited

Loading

alamb left a comment •

edited

Loading