Optimizing LogicalPlan with placeholders fails

[simonvandel]

Describe the bug

Optimizing a LogicalPlan with a placeholder fails with the error: "Placeholder type could not be resolved".

To Reproduce

 #[tokio::test]
    async fn fails() {
        let ctx = SessionContext::new();

        let df = ctx
            .read_empty()
            .unwrap()
            .with_column("a", lit(1))
            .unwrap()
            .filter(col("a").eq(placeholder(":0")))
            .unwrap();
        let logical_plan = df.logical_plan();

        // Fails with: Placeholder type could not be resolved
        ctx.state().optimize(logical_plan).unwrap();
    }

Expected behavior

The optimizer does not fail at the placeholder, but optimizes as much as it can with the info it has.

Additional context

No response

[alamb]

I agree this would be good to fix -- cc @kallisti-dev who I think is working on something related in IOx

[alamb]

@appletreeisyellow are you willing to look at this ticket?

[appletreeisyellow]

Yes!

[alamb]

Note there is a function that infers placeholder types: https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.Expr.html#method.infer_placeholder_types

Maybe we need to call it prior to the optimizer being run 🤔

[alamb]

Maybe this was fixed in #8977 by @kallisti-dev

[alamb]

Hi @simonvandel -- I wonder if you can provide some context about your expected behavior

The optimizer does not fail at the placeholder, but optimizes as much as it can with the info it has.

It looks like the change in #9073 from @appletreeisyellow assumes the flow is:

1. Create a logical plan
2. Provide the parameters
3. Run the optimizer

The description on this ticket implies your flow would be

1. Create the logical plan
2. Run the optimizer
3. Provide the parameters

Is this correct? Is there a reason you want to run the optimizer prior to providing parameter values?

The reason I ask is that depending on what you are trying to do the changes/solution may be different.

[simonvandel]

Hi @alamb

I sometimes have workloads where the time taken to plan a query dominates the time to actually perform the query. This can happen in the trivial case where the TableProviders returned no data.

Let's say that it's the same logical plan being used for every workload, just with different parameters. So then, my idea was to generate a logical plan with parameter placeholders, optimize that, and cache it. Then in each actual request, pull the optimized logical plan, and replace the placeholders with actual values.

The idea was that optimizing the final logical plan where the placeholders were replaced, would be very fast. This might have been naive.

So short story short, I wanted to cache optimized logical plans so they can be reused with different parameters.

Note that the optimizer is now quite a bit faster with the latest PRs, so the optimization time might not be a problem anymore. But I can still imagine the use case being relevant.

[alamb]

Thank you @simonvandel -- that makes total sense.

FWIW the physical optimizer passes to create an Execution Plan still do non trivial work even after the LogicalPlan is created.

I agree this usecase is reasonable one where running the optimizer takes non trivial time compared to query execution time. In fact this was the original usecase for parameterized queries in OLTP engines where the cost of planning dominated the cost of actually running the query so reusing a prepared statement was an important optimization

The usecase is much less common in classic analytic systems as the query execution time was often so much more than even 10s of ms of planning time.

However, as analytics is pushed everywhere, planning time is more important I think the fact that the DataFusion optimizer is so slow makes this even more pronounced. Ergo I think making planning faster via #5637 is very important

I am pretty sure from our (InfluxData)'s perspective, the prepared statement usecase is not much of a priority (especially compared to making planning overall faster), so @appletreeisyellow likely can't spend a lot of time on this issue (though maybe she feels differently).

Thus, what I suggest is we polish up #9073 which improves the error messages and then we can leave this particular ticket open for anyone else who might have the usecase and wants to improve it

[appletreeisyellow]

I am pretty sure from our (InfluxData)'s perspective, the prepared statement usecase is not much of a priority (especially compared to making planning overall faster), so @appletreeisyellow likely can't spend a lot of time on this issue (though maybe she feels differently).

+1. I'm align with @alamb's comment and leave this issue open for anyone else who wants to improve

[alamb]

I couldn't help myself -- here is a PR that stops copying so much during logical planning: #10016

(by the 38.0.0 release I expect the overall speed of DataFusion planning to be significantly improved)