Remove duplicate function name in its aliases list #10661

goldmedal · 2024-05-25T09:25:49Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

yes
Also checked starting the CLI with the DEBUG level.

RUST_LOG=DEBUG ./target/debug/datafusion-cli

Are there any user-facing changes?

No

goldmedal · 2024-05-25T10:19:12Z

datafusion/optimizer/src/push_down_limit.rs

@@ -65,7 +65,6 @@ impl OptimizerRule for PushDownLimit {
        };

        let Limit { skip, fetch, input } = limit;
-        let input = input;


I'm not sure why the clippy check in CI didn't detect this but it's showed when I ran clippy in my local.

goldmedal · 2024-05-25T10:20:10Z

datafusion/core/src/execution/context/mod.rs

            }
        }

        for function in all_default_aggregate_functions() {
            let udaf = new_state.register_udaf(function).unwrap();
            if let Some(udaf) = udaf {
-                assert!(false, "Function {} already registered", udaf.name());
+                unreachable!("Function {} already registered", udaf.name());


Clippy suggests that I use panic or unreachable instead of assert. I'm not sure which one is better.

alamb · 2024-05-25T10:29:57Z

datafusion/core/src/execution/context/mod.rs

+            function_factory: None,
+        };
+
+        for function in all_default_functions() {


This is a nice test -- though it is basically a copy of SessionContext::new

I wonder if we could make a test that is a bit simpler for example by creating a new
https://docs.rs/datafusion/latest/datafusion/execution/registry/struct.MemoryFunctionRegistry.html

Something like (untested)

let registry = MemoryFunctionRegistry::new(); for function in all_default_array_functions() { let existing_function = new_state.register_udf(function); assert!(existing_function.is_none(), "{} was already registered", function.name() } // and similarly for the aggregate and array functins

🤔

alamb · 2024-05-25T10:30:24Z

Thank you @goldmedal 🙏

goldmedal · 2024-05-25T12:23:17Z

Thanks, @alamb!

Actually, I tried to add some tests for MemoryFunctionRegistry before I fixed it, but I found everything was fine. I guess the reason is that we don't insert aliases when using MemoryFunctionRegistry#register_udf.

datafusion/datafusion/execution/src/registry.rs

Lines 174 to 176 in 4709fc6

    
           fn register_udf(&mut self, udf: Arc<ScalarUDF>) -> Result<Option<Arc<ScalarUDF>>> { 
        
               Ok(self.udfs.insert(udf.name().to_string(), udf)) 
        
           }

Then, I found that the aliases are only inserted by SessionState in

datafusion/datafusion/core/src/execution/context/mod.rs

Lines 2278 to 2283 in 4709fc6

    
           fn register_udf(&mut self, udf: Arc<ScalarUDF>) -> Result<Option<Arc<ScalarUDF>>> { 
        
               udf.aliases().iter().for_each(|alias| { 
        
                   self.scalar_functions.insert(alias.clone(), udf.clone()); 
        
               }); 
        
               Ok(self.scalar_functions.insert(udf.name().into(), udf)) 
        
           }

That's why I added tests for SessionState.

I'm not sure, but maybe we should also insert aliases in MemoryFunctionRegistry. What do you think?

alamb · 2024-05-26T09:52:15Z

I'm not sure, but maybe we should also insert aliases in MemoryFunctionRegistry. What do you think?

I think this would make sense -- though maybe we can do it as a follow on PR

alamb

Thank you for this find @goldmedal -- very cool. I think we can simplify the test a bit which would be good to consider.

alamb · 2024-05-26T09:57:01Z

datafusion/core/src/execution/context/mod.rs

@@ -2860,6 +2863,57 @@ mod tests {
        Ok(())
    }

+    #[tokio::test]
+    async fn test_register_default_functions() -> Result<()> {
+        let config = SessionConfig::new();


While I understand the proposal to test with code like SessionState::new , I think it is problematic because if SessionState::new is changed, then this test will no longer match what it is doing.

I think the core of this test is verifying that there are no duplicate names in the alias lists.

Thus, perhaps we could write tests for all_default_functions, etc like

let mut names = HashSet::new(); for func in all_default_functions() { assert(names.insert(func.name()).is_none(), "func.name duplicated") for alias in func.aliases() { assert(names.insert(alias).is_none(), "alias duplicated") } }

What do you think?

Good idea. I'll add this test and remove the test for SessionState::new. Thanks

goldmedal · 2024-05-26T13:06:47Z

datafusion/functions-aggregate/src/lib.rs

+        let mut names = HashSet::new();
+        for func in all_default_aggregate_functions() {
+            assert!(
+                names.insert(func.name().to_string().to_lowercase()),


I think the function name and alias should be case-insensitive. Therefore, lowercase them here.

goldmedal · 2024-05-26T14:23:40Z

I'm confused about the new median function. I'm not sure why the name is case-sensitive.

Planning query: Plan("Invalid function 'median'.\nDid you mean 'MEDIAN'?\n\n

I removed the duplicate name as I did for other functions in 4086688. However, the planner can't resolve the lowercase function name. I have tried and confirmed that other functions (e.g., first_value and approx_median) are case-insensitive. Only median is case-sensitive.

Do you have any idea? @alamb

I found this function was created in #10644.
Gentle ping @jayzhan211 , do you have any idea about it?
Thanks

goldmedal · 2024-05-26T15:16:44Z

I think I got it now. In #10644, MEDIAN was removed from the AggregationFunction list and reimplemented as a UDAF. It looks like the list of aggregation functions handles some name mapping for case insensitivity.

See:

datafusion/datafusion/expr/src/aggregate_function.rs

Line 117 in 52c4f3c

FirstValue => "FIRST_VALUE",

datafusion/datafusion/expr/src/aggregate_function.rs

Line 171 in 52c4f3c

"first_value" => AggregateFunction::FirstValue,

That's why we don't need to add a lowercase alias for FIRST_VALUE in first_last.rs. It seems FIRST_VALUE has been redesigned as a UDAF but hasn't been removed from the AggregationFunction list. I'm not sure if this is an issue.

I reverted the change for median and removed the name testing for case insensitivity. However, I think adding a lowercase alias for MEDIAN is a workaround to handle the function name case insensitivity issue for UDAF. Maybe we need a formal way to handle this, but I'm not sure.

jayzhan211 · 2024-05-27T00:37:11Z

That's why we don't need to add a lowercase alias for FIRST_VALUE in first_last.rs. It seems FIRST_VALUE has been redesigned as a UDAF but hasn't been removed from the AggregationFunction list. I'm not sure if this is an issue.

first_value is not added in alias because the function is not fully converted to UDAF yet, so it will fail some tests. #10648 has alias for lower case

so as approx_median since it is not UDAF yet.

I think adding a lowercase alias for MEDIAN is a workaround to handle the function name case insensitivity issue for UDAF

Why adding a lowercase alias is a workaround for UDAF? Without alias, we will need to check the lowercase in many places.

goldmedal · 2024-05-27T11:37:11Z

first_value is not added in alias because the function is not fully converted to UDAF yet, so it will fail some tests. #10648 has alias for lower case

so as approx_median since it is not UDAF yet.

Thanks for the information! @jayzhan211

I think adding a lowercase alias for MEDIAN is a workaround to handle the function name case insensitivity issue for UDAF

Why adding a lowercase alias is a workaround for UDAF? Without alias, we will need to check the lowercase in many places.

Since I thought we needed to list all case patterns for the function name (e.g., Median, MEDian), I called it a workaround. However, I found I was wrong. After some testing, I discovered something interesting. For DataFusion, if we query a function

SELECT Median(xx) FROM tt

The function call will be transferred to median. Even query it use

SELECT MEDIAN(xx) FROM tt

It will also be changed to median. The uppercase name is only be matched when invoke this function with qutoes

SELECT "MEDIAN"(xx) FROM tt

So, how about setting UDAF names to lowercase (e.g., median, first_value)? This way, we don't need another uppercase alias. What do you think?

…iple-register

alamb

Tahnk you @goldmedal and @jayzhan211 -- I think this is a really nice improvement. Not only does it fix the bug it also makes the code easier to work with and avoids introducing the same regression again

I took the liberty of merging up from main to resolve a merge conflict

alamb · 2024-05-27T12:25:11Z

So, how about setting UDAF names to lowercase (e.g., median, first_value)? This way, we don't need another uppercase alias. What do you think?

This makes sense to me

Shall we file a follow on ticket for it? I don't think we need to fix it in this PR (as this PR doesn't make the situation any better or worse, from what I can tell).

goldmedal · 2024-05-27T13:01:38Z

Shall we file a follow on ticket for it? I don't think we need to fix it in this PR (as this PR doesn't make the situation any better or worse, from what I can tell).

Thanks @alamb, I agree with you. Fixing it in a follow-up ticket is better.

jayzhan211 · 2024-05-28T12:31:27Z

Thanks, @goldmedal and @alam. I think we can merge this

goldmedal · 2024-05-28T12:50:57Z

Thanks again @alamb @jayzhan211

alamb · 2024-05-28T13:02:09Z

Shall we file a follow on ticket for it? I don't think we need to fix it in this PR (as this PR doesn't make the situation any better or worse, from what I can tell).

Thanks @alamb, I agree with you. Fixing it in a follow-up ticket is better.

Filed #10695 to track

* remove duplicate name in function name aliases for array-function * add test for registering default functions * rename test case * add tests for agg and core function and refactor the test * remove unused mut * add comments for public function * cargo fmt * fix clippy * fix missing list_element * fix clippy * remove duplicate aliase name for median * add test for function list and remove the test for SessionState * remove the debug message * revert the change for medain and remove case insensitive test --------- Co-authored-by: Andrew Lamb <[email protected]>

github-actions bot added core Core DataFusion crate optimizer Optimizer rules labels May 25, 2024

goldmedal commented May 25, 2024

View reviewed changes

alamb reviewed May 25, 2024

View reviewed changes

alamb reviewed May 26, 2024

View reviewed changes

goldmedal added 12 commits May 26, 2024 19:57

remove duplicate name in function name aliases for array-function

2113221

add test for registering default functions

83bd7bc

rename test case

75ea355

add tests for agg and core function and refactor the test

b5e6c60

remove unused mut

39714a5

add comments for public function

3973e09

cargo fmt

797167a

fix clippy

1910185

fix missing list_element

36a96e4

fix clippy

8fc2a1a

remove duplicate aliase name for median

4086688

add test for function list and remove the test for SessionState

8c2c828

goldmedal force-pushed the bugfix/10658-udf-multiple-register branch from 340329d to 8c2c828 Compare May 26, 2024 13:03

github-actions bot removed the core Core DataFusion crate label May 26, 2024

goldmedal commented May 26, 2024

View reviewed changes

remove the debug message

47abceb

revert the change for medain and remove case insensitive test

683af52

Merge remote-tracking branch 'apache/main' into bugfix/10658-udf-mult…

7b1de1e

…iple-register

alamb approved these changes May 27, 2024

View reviewed changes

jayzhan211 merged commit 98cd19e into apache:main May 28, 2024
23 checks passed

goldmedal deleted the bugfix/10658-udf-multiple-register branch May 28, 2024 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove duplicate function name in its aliases list #10661

Remove duplicate function name in its aliases list #10661

goldmedal commented May 25, 2024

goldmedal May 25, 2024

goldmedal May 25, 2024 •

edited

Loading

alamb May 25, 2024

alamb commented May 25, 2024

goldmedal commented May 25, 2024

alamb commented May 26, 2024

alamb left a comment

alamb May 26, 2024

goldmedal May 26, 2024

goldmedal May 26, 2024

goldmedal commented May 26, 2024 •

edited

Loading

goldmedal commented May 26, 2024

jayzhan211 commented May 27, 2024 •

edited

Loading

goldmedal commented May 27, 2024

alamb left a comment

alamb commented May 27, 2024

goldmedal commented May 27, 2024 •

edited

Loading

jayzhan211 commented May 28, 2024

goldmedal commented May 28, 2024

alamb commented May 28, 2024

Remove duplicate function name in its aliases list #10661

Remove duplicate function name in its aliases list #10661

Conversation

goldmedal commented May 25, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

goldmedal May 25, 2024

Choose a reason for hiding this comment

goldmedal May 25, 2024 • edited Loading

Choose a reason for hiding this comment

alamb May 25, 2024

Choose a reason for hiding this comment

alamb commented May 25, 2024

goldmedal commented May 25, 2024

alamb commented May 26, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb May 26, 2024

Choose a reason for hiding this comment

goldmedal May 26, 2024

Choose a reason for hiding this comment

goldmedal May 26, 2024

Choose a reason for hiding this comment

goldmedal commented May 26, 2024 • edited Loading

goldmedal commented May 26, 2024

jayzhan211 commented May 27, 2024 • edited Loading

goldmedal commented May 27, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb commented May 27, 2024

goldmedal commented May 27, 2024 • edited Loading

jayzhan211 commented May 28, 2024

goldmedal commented May 28, 2024

alamb commented May 28, 2024

goldmedal May 25, 2024 •

edited

Loading

goldmedal commented May 26, 2024 •

edited

Loading

jayzhan211 commented May 27, 2024 •

edited

Loading

goldmedal commented May 27, 2024 •

edited

Loading