Skip to content

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

Closed
alamb wants to merge 1 commit into
apache:masterfrom
alamb:case_insensitive_functions
Closed

ARROW-11822: [Rust][Datafusion] Support case sensitive comparisons for functions and aggregates#9827
alamb wants to merge 1 commit into
apache:masterfrom
alamb:case_insensitive_functions

Conversation

@alamb

Copy link
Copy Markdown
Contributor

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

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.

@alambalamb changed the title ARROW-11822: [Rust][Datafusion] Support case sensitive comparisons for functionsARROW-11822: [Rust][Datafusion] Support case sensitive comparisons for functions and aggregatesMar 28, 2021
@github-actions

Copy link
Copy Markdown

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #9827 (1440a98) into master (05a507c) will increase coverage by 0.01%.
The diff coverage is 98.63%.

Impacted file tree graph

@@ Coverage Diff @@## master #9827 +/- ##
==========================================
+ Coverage 82.42% 82.43% +0.01% 
==========================================
Files 252 252 Lines 58977 59043 +66 ==========================================
+ Hits 48609 48674 +65 - Misses 10368 10369 +1 
Impacted FilesCoverage Δ
rust/datafusion/src/sql/planner.rs83.55% <83.33%> (+0.07%)⬆️
rust/datafusion/src/execution/context.rs91.43% <100.00%> (+0.56%)⬆️
rust/datafusion/src/physical_plan/aggregates.rs90.90% <100.00%> (ø)
rust/parquet/src/encodings/encoding.rs94.86% <0.00%> (-0.20%)⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05a507c...1440a98. Read the comment docs.

@alamb
alambforce-pushed the case_insensitive_functions branch from 1440a98 to e0c73d2CompareApril 4, 2021 09:48
@alamb

alamb commented Apr 5, 2021

Copy link
Copy Markdown
ContributorAuthor

@jorgecarleitaojorgecarleitao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. this beauty in just less than 20 LOC. 💯

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@alamb@codecov-io@returnString@jorgecarleitao