Skip to content

Complete support for Expr --> String  #9726

New issue
New issue
Closed
#9805
Closed
Complete support for Expr --> String #9726
#9805
Labels
enhancementNew feature or request

Description

@alamb
alamb
opened on Mar 21, 2024

Part of #9494

Is your feature request related to a problem or challenge?

This ticket tracks the remaining work to complete #9495

As @devinjdangelo says in #9494 (comment)

We now have a solid foundation for converting Exprs --> SQL (see #9495 for why this is valuable).

It should now be straightforward to add support for the remaining Expr types and doing so is a great way to get more familiar with DataFusion's core data structures and optimization algorithms without already having expertise in database internals.

Describe the solution you'd like

The basic task is to:

  1. Pick one (or a few) of the expressions below
  2. Create a PR to Implement the Expr --> AST reverse code (in this match statement)
  3. Add a test (for example, here)

Here is the remaining list of exprs (just note on the ticket which you plan to work on)

Describe alternatives you've considered

No response

Additional context

No response

Activity

  1. alamb
    added
    enhancementNew feature or request
    good first issueGood for newcomers
    on Mar 21, 2024
  2. alamb
    changed the title [-]Implement support for `Expr --> String `[/-] [+]Complete support for `Expr --> String `[/+] on Mar 21, 2024

  3. Lordworms commented on Mar 22, 2024

    @Lordworms
    Lordworms
    on Mar 22, 2024
    Contributor

    take IsTrue, IsFalse, IsUnknown

  4. sebastian2296 commented on Mar 22, 2024

    @sebastian2296
    sebastian2296
    on Mar 22, 2024
    Contributor

    I'd like to work on isNotNull, Not and Between

  5. yyy1000 commented on Mar 23, 2024

    @yyy1000
    yyy1000
    on Mar 23, 2024
    Contributor

    take ScalarFunction and InList

  6. yyy1000 commented on Mar 25, 2024

    @yyy1000
    yyy1000
    on Mar 25, 2024
    Contributor

    take Case

  7. Weijun-H commented on Mar 26, 2024

    @Weijun-H
    Weijun-H
    on Mar 26, 2024
    Member

    take Like

  8. kevinmingtarja commented on Mar 26, 2024

    @kevinmingtarja
    kevinmingtarja
    on Mar 26, 2024
    Contributor

    take Sort, Exists

  9. pratikpugalia commented on Mar 26, 2024

    @pratikpugalia
    pratikpugalia
    on Mar 26, 2024

    I'd like to work on Negative

  10. alamb
    closed this as completedin #9805on Mar 27, 2024
  11. alamb
    reopened this on Apr 1, 2024

  12. alamb commented on Apr 1, 2024

    @alamb
    alamb
    on Apr 1, 2024
    ContributorAuthor

    It looks like there are a few things not yet complete in https://github.com/apache/arrow-datafusion/blob/main/datafusion/sql/src/unparser/expr.rs#L52

    • Window functions
    • user defined functions
    • (maybe more)

  13. yyy1000 commented on Apr 1, 2024

    @yyy1000
    yyy1000
    on Apr 1, 2024
    Contributor

    take SimilarTo, IsNotTrue, IsNotUnknown,Negative

  14. yyy1000 commented on Apr 1, 2024

    @yyy1000
    yyy1000
    on Apr 1, 2024
    Contributor

    Here is a list that has not been done yet, for interested contributor to better view:

    • GetIndexedField
    • ScalarVariable
    • TryCast
    • Sort
    • Exists
    • Wildcard
    • GroupingSet
    • Placeholder
    • OuterReferenceColumn
    • Unnest
    • WindowFunction

  15. 7 remaining items

  16. yyy1000 commented on Apr 16, 2024

    @yyy1000
    yyy1000
    on Apr 16, 2024
    Contributor

    Hi @devanbenz , I think ScalarVariable has not been done yet and you could have a try, though it may not be possible.🤔
    I think maybe it should be converted to ScalarValue first, then call scalar_to_sql function.

  17. alamb commented on Apr 16, 2024

    @alamb
    alamb
    on Apr 16, 2024
    ContributorAuthor

    Here are some thoughts to help people figure out what SQL matches to what expression:

    • GetIndexedField --> `SELECT col['field_name']
    • ScalarVariable --> Not sure (maybe @@foo@@?
    • TryCast * --> CAST (COL AS <datatype>>
    • Sort --> col ASC or col DESC NULLS FIRST
    • Exists --> col exists (SELECT 1 from foo)
    • Wildcard --> COUNT(*)
    • GroupingSet --> Not sure
    • Placeholder --> SELECT $1
    • OuterReferenceColumn --> not sure (I think this is the same as a column)
    • Unnest --> UNNEST(SELECT col FROM bar)
    • WindowFunction --> first_value(x ORDER BY y PARTITION BY z)

  18. devanbenz commented on Apr 23, 2024

    @devanbenz
    devanbenz
    on Apr 23, 2024
    Contributor

    Hi @devanbenz , I think ScalarVariable has not been done yet and you could have a try, though it may not be possible.🤔 I think maybe it should be converted to ScalarValue first, then call scalar_to_sql function.

    Taking a look at this today. Will give it a try and update this thread with any thoughts, observations, or problems I run in to :)

  19. devanbenz commented on Apr 23, 2024

    @devanbenz
    devanbenz
    on Apr 23, 2024
    Contributor

    Here are some thoughts to help people figure out what SQL matches to what expression:

    • GetIndexedField --> `SELECT col['field_name']
    • ScalarVariable --> Not sure (maybe @@foo@@?
    • TryCast * --> CAST (COL AS <datatype>>
    • Sort --> col ASC or col DESC NULLS FIRST
    • Exists --> col exists (SELECT 1 from foo)
    • Wildcard --> COUNT(*)
    • GroupingSet --> Not sure
    • Placeholder --> SELECT $1
    • OuterReferenceColumn --> not sure (I think this is the same as a column)
    • Unnest --> UNNEST(SELECT col FROM bar)
    • WindowFunction --> first_value(x ORDER BY y PARTITION BY z)

    @alamb I beleive ScalarVariable would just be something like @variable.

  20. alamb commented on Apr 23, 2024

    @alamb
    alamb
    on Apr 23, 2024
    ContributorAuthor

    Thanks @devanbenz !

  21. devanbenz commented on May 6, 2024

    @devanbenz
    devanbenz
    on May 6, 2024
    Contributor

    @alamb if you could please take a look at apache/datafusion-sqlparser-rs#1260 -- it's linked to this issue :)

  22. alamb commented on May 6, 2024

    @alamb
    alamb
    on May 6, 2024
    ContributorAuthor

    @alamb if you could please take a look at sqlparser-rs/sqlparser-rs#1260 -- it's linked to this issue :)

    Thanks @devanbenz -- can you explain how this is related to this issue? (Sorry I am sure it is obvious but I can't keep everything in my head anymore )

  23. devanbenz commented on May 6, 2024

    @devanbenz
    devanbenz
    on May 6, 2024
    Contributor

    @alamb if you could please take a look at sqlparser-rs/sqlparser-rs#1260 -- it's linked to this issue :)

    Thanks @devanbenz -- can you explain how this is related to this issue? (Sorry I am sure it is obvious but I can't keep everything in my head anymore )

    All good - this is for the Unparser expr_to_sql match statement

    datafusion/datafusion/sql/src/unparser/expr.rs

    Line 391 in 89443bf

    Expr::ScalarVariable(_, _) => {
    I need to have a supported ast::Expr type for ScalarVariable from sql-parser. 👍

  24. alamb commented on May 7, 2024

    @alamb
    alamb
    on May 7, 2024
    ContributorAuthor

    It seems to me that Expr::ScalarVariable currently is parsed from ast::Identifier (that starts with @)-- perhaps instead of adding a new ast node, you could just create an equivalent ast::Identifier 🤔

    datafusion/datafusion/sql/src/expr/identifier.rs

    Lines 28 to 43 in accce97

    pub(super) fn sql_identifier_to_expr(
    &self,
    id: Ident,
    schema: &DFSchema,
    planner_context: &mut PlannerContext,
    ) -> Result<Expr> {
    if id.value.starts_with('@') {
    // TODO: figure out if ScalarVariables should be insensitive.
    let var_names = vec![id.value];
    let ty = self
    .context_provider
    .get_variable_type(&var_names)
    .ok_or_else(|| {
    plan_datafusion_err!("variable {var_names:?} has no type information")
    })?;
    Ok(Expr::ScalarVariable(ty, var_names))

  25. devanbenz commented on May 7, 2024

    @devanbenz
    devanbenz
    on May 7, 2024
    Contributor

    It seems to me that Expr::ScalarVariable currently is parsed from ast::Identifier (that starts with @)-- perhaps instead of adding a new ast node, you could just create an equivalent ast::Identifier 🤔

    datafusion/datafusion/sql/src/expr/identifier.rs

    Lines 28 to 43 in accce97

    pub(super) fn sql_identifier_to_expr(
    &self,
    id: Ident,
    schema: &DFSchema,
    planner_context: &mut PlannerContext,
    ) -> Result<Expr> {
    if id.value.starts_with('@') {
    // TODO: figure out if ScalarVariables should be insensitive.
    let var_names = vec![id.value];
    let ty = self
    .context_provider
    .get_variable_type(&var_names)
    .ok_or_else(|| {
    plan_datafusion_err!("variable {var_names:?} has no type information")
    })?;
    Ok(Expr::ScalarVariable(ty, var_names))

    Sounds good - I'll have some time to come back to this in the next couple days! Will modify per your suggestion :)

  26. alamb commented on May 15, 2024

    @alamb
    alamb
    on May 15, 2024
    ContributorAuthor

    I filed tickets for the remaining issues

    These ones are likely straightforward.

    The others are likely more involved as I don't know how to make a reproducer

  27. alamb
    removed
    good first issueGood for newcomers
    on May 15, 2024

  28. alamb commented on May 20, 2024

    @alamb
    alamb
    on May 20, 2024
    ContributorAuthor

    With the completion of #10555 from @xinlifoobar I think this epic is now done!

  29. alamb
    closed this as completedon May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions