Skip to content

Conversation

@2010YOUY01
Copy link
Contributor

Which issue does this PR close?

Part of #6396.

Rationale for this change

Currently BuiltinScalarFunction is using the derived Debug trait to display function names, it might get slightly different from the actual function names used in the SQL statements.

DataFusion CLI v25.0.0
❯ select arrow_typeof(1);
+-----------------------+
| arrowtypeof(Int64(1)) |
+-----------------------+
| Int64                 |
+-----------------------+

FromStr is used to bind function string in SQL statements to Enum BuiltinScalarFunction variants, it would be better to implement Display on BuiltinScalarFunction as the "reverse" of FromStr

What changes are included in this PR?

Are these changes tested?

Added one unit test to make sure Display is consistent with FromStr

Are there any user-facing changes?

No

@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels May 25, 2023
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @2010YOUY01 -- this looks great to me

It is consistent with postgres as well 👍

postgres=# select starts_with('foo', 'bar');
 starts_with
-------------
 f
(1 row)

}
}

// Should not be reached
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Maybe we should put an assert here? I'll do so as a follow on PR

/// Mapping between SQL function names to `BuiltinScalarFunction` types.
/// Note that multiple SQL function names can represent the same `BuiltinScalarFunction`. These are treated as aliases.
/// In case of such aliases, the first SQL function name in the vector is used when displaying the function.
static ref NAME_TO_FUNCTION: Vec<(&'static str, BuiltinScalarFunction)> = vec![
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really nice solution and writeup -- thank you @2010YOUY01

I am slightly concerned about having to do a linear search for each function name, but I don't think it is a big deal and I will propose a follow on PR to improve the situation

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

Labels

core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants