[Epic] Complete pulling out special SQL planning from the Sql Parser about datafusion HOT 18 CLOSED

See datafusion-contrib/datafusion-functions-json#26 - support for custom SQL operators in datafusion-functions-json using #11208.

from datafusion.

hello @alamb Just checking in on the remaining tasks. Is there anything specific we're waiting on before we create issues ? If we're all set, i would be happy to jump in and get started to pick up few tasks.

Hi @dharanad I don't think there is anything from my perspective. Thank you for offering

In fact it seems as if @xinlifoobar has already started with #11215 ❤️

from datafusion.

I've created issues for a couple of tasks. Please let me know if you think anything needs updating in the descriptions. I'm new here and learning from shadowing the experienced folks

• date_part : #11220
• create_struct : #11221
• create_named_struct : #11222
• sql_overlay_to_expr : #11223

from datafusion.

FWIW in general @dharanad I have had the best luck with writing a description on tickets that requires as little context as possible (aka distill down what is needed into the the description, rather than assuming the new contributor will read the epic and get all the backstory)

The rationale for this duplication is to lower the barrier to new contrbutors

from datafusion.

Given how much UserDefinedSQLPlanner is being used for existing stuff within datafusion, perhaps it should be called just SQLPlanner or CustomSQLPlanner?

I agree

Or maybe something like ExprPlanner 🤔 as it is being used to plan specific exprs.

from datafusion.

Given #11220 and #11243, those are very similar APIs with UDF plans. I am trying to draft an API, e.g.,

    // Plan the user defined function, returns origin expression arguments if not possible
    fn plan_udf(
        &self,
        _sql: &sqlparser::ast::Expr,
        args: Vec<Expr>,
    ) -> Result<PlannerResult<Vec<Expr>>> {
        Ok(PlannerResult::Original(args))
    }

to uniform the usages.

I have created a draft PR #11263 to discuss this. The flaw here is that the parameter sql is partially borrowed and has to be cloned at the very beginning. Maybe we should consider using references if possible.

from datafusion.

Given #11220 and #11243, those are very similar APIs with UDF plans. I am trying to draft an API, e.g.,

    // Plan the user defined function, returns origin expression arguments if not possible
    fn plan_udf(
        &self,
        _sql: &sqlparser::ast::Expr,
        args: Vec<Expr>,
    ) -> Result<PlannerResult<Vec<Expr>>> {
        Ok(PlannerResult::Original(args))
    }

to uniform the usages.

I have created a draft PR #11263 to discuss this. The flaw here is that the parameter sql is partially borrowed and has to be cloned at the very beginning. Maybe we should consider using references if possible.

Eventually, I made this #11263, please let me know your thoughts. Thanks :)

CC @jayzhan211 @dharanad @alamb

from datafusion.

I think we can claim we are done 🎉

thanks everyone

from datafusion.

#11208 allows user defined sql planners to be defined.

from datafusion.

hello @alamb Just checking in on the remaining tasks. Is there anything specific we're waiting on before we create issues ?
If we're all set, i would be happy to jump in and get started to pick up few tasks.

from datafusion.

I've created issues for a couple of tasks. Please let me know if you think anything needs updating in the descriptions. I'm new here and learning from shadowing the experienced folks

thank you @dharanad -- this is very helpful 🙏

from datafusion.

FWIW in general @dharanad I have had the best luck with writing a description on tickets that requires as little context as possible (aka distill down what is needed into the the description, rather than assuming the new contributor will read the epic and get all the backstory)

The rationale for this duplication is to lower the barrier to new contrbutors

Thanks for the feedback! I really appreciate. You're right, making the ticket description concise and self-contained will definitely help reduce the barrier for new contributors. I'll update the description to include the necessary context. Thanks you

from datafusion.

Create issues for the remaining tasks, tried adding a description based on my understanding of the issue. Also update the same for the older ones

• sql_compound_identifier_to_expr: #11244
• sql_substring_to_expr : #11245
• sql_position_to_expr: #11246

from datafusion.

Given how much UserDefinedSQLPlanner is being used for existing stuff within datafusion, perhaps it should be called just SQLPlanner or CustomSQLPlanner?

from datafusion.

ExprPlanner sounds good.

from datafusion.

ExprPlanner sounds good.

Filed #11304

from datafusion.

I think we are pretty close to calling this done.

I just double checked and sql_compound_identifier_to_expr is the only thing that needs this treatment to remove the call to get_function_meta:

That appears to be the last issue
https://github.com/search?q=repo%3Aapache%2Fdatafusion+get_function_meta+path%3A%2F%5Edatafusion%5C%2Fsql%5C%2F%2F&type=code

from datafusion.

Filed #11473

from datafusion.