Improve round scalar function unparsing for Postgres

[sgrebnov]

Which issue does this PR close?

PR improves unparsing to generate valid sql for Postgres and round scalar function by enforcing NUMERIC data type for it.

Postgres requires NUMERIC data type for round as it does not support Float arguments that are produced by default. Right now unparsed sql fails with the error "Query Error function round(double precision, integer) does not exist" if round is used.

https://www.postgresql.org/docs/17/functions-math.html

round ( v numeric, s integer ) → numeric
Rounds v to s decimal places. Ties are broken by rounding away from zero.
round(42.4382, 2) → 42.44
round(1234.56, -1) → 1230

-- SQL Error [42883]: ERROR: function round(double precision, integer) does not exist
select round(CAST(1.23456 AS float), 2)

-- 1.23
select round(CAST(1.23456 AS numeric), 2)

What changes are included in this PR?

1. Support for dialects to override scalar functions unparsing via exposing scalar_function_to_sql_overrides
2. Refactored date_part scalar function unparsing to rely on scalar_function_to_sql_overrides approach above
3. Added logic to enforce NUMERIC for round scalar function unparsing for Postgres

Are these changes tested?

Unit test for round function unparsing for default dialect and Postgres

Are there any user-facing changes?

Unaprser will generate valid sql for round scalar function for Postgres dialect

[sgrebnov]

Alternative approach considered was removing casting altogether, but it seems less robust as the argument can be a complex expression.

[alamb]

Rather than checking after unparsing, maybe this code could check before unparsing -- as in get the type of Expr in args[0] and use that to decide if a cast is needed

[sgrebnov]

That is a good idea 👍. Unfortunately, we can't enforce NUMERIC before unparsing, as we operate on the Arrow DataType level, which includes FLOAT, DOUBLE, etc., and these types will be converted to FLOAT, DECIMAL, or DOUBLE PRECISION types and we can't enforce NUMERIC before unparsing. Since we require to do unparsing anyway, I think it is better to have all the relevant logic in the same place—right after unparsing.

[alamb]

Looks good to me -- thank you @sgrebnov

[alamb]

👍

Could you please add documentation here explaining what this function does / is used for?

[sgrebnov]

@alamb - thank you for pointing to this, missed this part, documented the scalar_function_to_sql_overrides

[alamb]

Rather than checking after unparsing, maybe this code could check before unparsing -- as in get the type of Expr in args[0] and use that to decide if a cast is needed

[alamb]

this is a nice refactor

[alamb]

👍

[goldmedal]

Thanks @sgrebnov It's a nice improvement. scalar_function_to_sql_overrides would be very helpful. I agree with @alamb's comment. It's better to add some doc for this method. Others LGTM. 👍

[phillipleblanc]

Thanks Sergei!

Sergei and I worked together on this PR in our fork before he raised this PR: spiceai#34

I wasn't happy with how we were handle differences in function unparsing as part of the core datafusion code. I initially thought we shouldn't deal with it at all in this code, and perform another pass over the SQL AST in our own codebase. But I came around to the idea that the functions in a database system is part of the dialect, so it does make sense to handle them.

However, I wanted an approach that would allow people to implement their own function unparsing for custom dialects, without requiring changes to the core datafusion code. I thought maybe having a function registry on the dialect trait would be useful (i.e. register a function by name, along with how to unparse it).

The approach Sergei came up with here is pretty elegant imo and still simple. I expect once we get into the 10s of functions we need to customize, it might start to get unwieldy and we may consider refactoring it further. But for now I'm happy with this approach.

[goldmedal]

However, I wanted an approach that would allow people to implement their own function unparsing for custom dialects, without requiring changes to the core datafusion code. I thought maybe having a function registry on the dialect trait would be useful (i.e. register a function by name, along with how to unparse it).

The approach Sergei came up with here is pretty elegant imo and still simple. I expect once we get into the 10s of functions we need to customize, it might start to get unwieldy and we may consider refactoring it further. But for now I'm happy with this approach.

Hi @phillipleblanc,

I believe this PR offers a valuable improvement by allowing a DataFusion function to be unparsed to another dialect's function. However, in my project, WrenAI, we face a similar scenario with custom functions used for unparsing. Our goal is to parse functions that are not part of DataFusion into the plan and then unparse them back to SQL.

I'm not entirely sure how Spiceai operates, but I assume you also perform some form of full pushdown things, correct? I'd like to share our use case with you.

In WrenAI, our process is as follows:

• Parse SQL into a Logical Plan
• Apply rewrite rules
• Unparse the rewritten plan back to DataFusion SQL
• Transpile the DataFusion SQL into other database SQL dialects using ibis-project and sqlglot

The challenge lies in parsing custom functions that aren't part of DataFusion's core into the logical plan. To address this, we've implemented a UDF called ByPassScalarUDF specifically for unparsing purposes.

You can view the implementation here:
https://github.com/Canner/wren-engine/blob/861f978df3438a1ba01d3518101de59bcffd1da8/wren-modeling-rs/core/src/mdl/function.rs#L15

At program startup, we can register the UDFs like this:

let ctx = SessionContext::new();
ctx.register_udf(ScalarUDF::new_from_impl(ByPassScalarUDF::new("date_diff", "bigint")));
ctx.register_udf(ScalarUDF::new_from_impl(ByPassScalarUDF::new("year", "bigint")));

Since this implementation only requires the function name and return type, we can easily define them in a file and register them from there. Once registered, the unparser can correctly output the function's SQL.

[sgrebnov]

@alamb, @goldmedal, @phillipleblanc - thank you for review, documented scalar_function_to_sql_overrides , really missed that part - will do better next time :)

[alamb]

THanks again everyone -- if there are more improvements to be made, let's make them as follow on PRs!