Thanks for opening an issue!

Do you have a use case for this?

For this specific issue, we removed casting because it was defeating some important pushdown optimizations in duckdb-spatial.

I found this when I got an overflow error from multiplying two large numbers as I describe.

To be specific, when multiplying multiple large probabilities in my record linkage library mismo.
See
NickCrews/mismo@357fa79#diff-1ec0582d16916ba2d4319891e339b61c8e19c5942fb4912be84f02e9d1fecd30R311-R313 (eg we have several columns which represent the odds from some feature, eg that the address or names match, and multiply them together. These odds can be quite large. See https://moj-analytical-services.github.io/splink/topic_guides/theory/fellegi_sunter.html#deriving-match-weights-from-m-and-u)

If we add the explicit cast only for when ibis compiles a floatXX literal to duckdb, would that manage to avoid the optimization issue?

If others run into this, the workaround I found was ibis.literal("12.34").cast(float)

You can remove the cast and specify the dtype when you define the literal

[ins] In [2]: ibis.literal(1, type='float')
Out[2]: 1.0

[ins] In [3]: ibis.literal(12.34, type='float')
Out[3]: 12.34

[ins] In [4]: ibis.literal('12.34', type='float')
Out[4]: 12.34

Unfortunately, all those result in the naked 12.34 (which duckdb interprets as decimal) in the generate SQL:

import ibis

ibis.to_sql(ibis.literal(1, type="float"), dialect="duckdb")  # SELECT 1.0 AS "1.0"
ibis.to_sql(ibis.literal(12.34, type='float'), dialect='duckdb')  # SELECT 12.34 AS "12.34"
ibis.to_sql(ibis.literal("12.34", type='float'), dialect='duckdb')  # SELECT 12.34 AS "12.34"
ibis.to_sql(ibis.literal("12.34").cast("float"), dialect="duckdb")  # SELECT CAST('12.34' AS DOUBLE) AS "Cast('12.34', float64)"
ibis.to_sql(ibis.literal(12.34).cast("float"), dialect="duckdb")  # SELECT 12.34 AS "12.34"

In order to actually get ibis to generate the cast-to-float, you need to start with something that ibis doesn't think is a float, eg the ibis.literal("12.34") which ibis thinks has dtype of string.

Here is a little test that I would expect to pass for any expression and backend. Could we add this to our tests, where hypothesis generates expressions for us?

import ibis
from ibis.backends.sql import SQLBackend
from ibis.expr import datatypes as dt


def assert_type_roundtrips(e: ibis.ir.Value, backend: SQLBackend):
    type_in_backend_string: str = backend.execute(e.typeof())
    type_in_backend_ibis: dt.DataType = backend.compiler.type_mapper.from_string(
        type_in_backend_string
    )
    assert e.type() == type_in_backend_ibis, (e.type(), type_in_backend_ibis)


e = ibis.literal(12.34)
backend = ibis.duckdb.connect()
assert_type_roundtrips(e, backend)
# AssertionError: (Float64(nullable=True), Decimal(precision=4, scale=2, nullable=True))

If you can come up with a way to not break existing optimizations on DuckDB and get that test to pass, you're more than welcome to try adding it to the test suite.

That test used to pass before we disabled literal casting on DuckDB. We made the concession because we deemed it more important to enable the optimization that was defeated by casting.

OK, that tradeoff makes sense that we want to prioritize the optimization more, I bet more people are affected by the optimization than by the bug that I experienced, and my bug has a workaround.

I am willing to try to put in a bit of effort to solve this, if it doesn't turn into a total rabbit hole.

Links to the perf issues:

• original bug: bug(geospatial): casting of literals preventing filter push down to parquet reader #9662
• PR that fixed it: perf(duckdb): remove numeric literal casts when compiling sql #9664

@cpcloud in that linked PR, you mention what you think the long-term solution should be:

Longer term, I think we should probably support two kinds of literals:

1. A ops.TypedLiteral when a type is specified in ibis.literal, which will be cast to the specific type
2. ops.Literal would then be a literal that has type information but avoids casting. This is what op construction would use.

Can you give some example of this? If for number 2 you are saying that Ops.Multiply(1, 12.34) should avoid casting and compile to 1 * 12.34, then we would still suffer from ibis thinking this is a float64 but it getting evaluated as a decimal on the backend.

Alternatively, instead of having two different flavors of Ops at construction time, could we do it through re-write rules at compile time?

1. in expressions like ops.Equal(x, ops.Literal(12.34)), we can avoid the cast, because the overall type of boolean doesn't care about if the 12.34 is a float or decimal.
2. but, in expressions like ops.Add(x, ops.Literal(12.34)), where the overall type is numeric, we DO need the cast?

It might be death by a thousand cuts to properly enumerate all the rules for when the type is important and when it is not. What about if by default we made it so that we compiled the type cast, but just special cased the boolean comparison Ops to not include the cast during compilation? Eg

1. we kept pretty much all our implementation for building the expression tree the same.
2. we added a new ops.UntypedLiteral
3. we added a rewrite rule so that ops.Equal(x, ops.Literal(12.34)) get rewritten to ops.Equal(x, ops.UntypedLiteral(12.34))
4. ops.Literal, when compiled, always includes the cast, ops.UntypedLiteral does not.

My thought here is that by default we should try to have explicit casting so that assert_type_roundtrips always holds, and then only special case away from this when we find there are optimizations etc that we want to keep.

It also looks like in that PR, there is no automatic test to make sure that the filter pushdown happens, is that right? You were just manually running EXPLAIN <query> on the query from the original issue?

#10933 implements the rewrite solution that I describe above.