Recursive unnest

[duongcongtoai]

Which issue does this PR close?

Closes #10660.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

[jayzhan211]

use swap_remove(0) to avoid clone

[jayzhan211]

We could avoid clone in L297 too.

[jayzhan211]

Would transform List / Struct separately help readability here?

something like

transform_bottom_unnest() {
 // common logic
 fn transform();

 list:
     transform_list // list specific logic
 struct:
     transform_struct // struct specific logic (only unnest with the top level)
}

[alamb]

I plan to review this tomorrow

[alamb]

Thank you @duongcongtoai -- I think this PR is quite close. In my opinion it only needs a few more tests and we should be able to merge it.

Thanks @jayzhan211 for the review

[alamb]

should we file a ticket to track this?

[duongcongtoai]

I moved this FIXME to sqllogictest, ticket created: #11198

[alamb]

would it be possible to please leave the original test the same (so we can verify the existing behavior is unchanged) and add new tests for the newly added features?

[duongcongtoai]

i revert the change on this table and created a new one

[alamb]

Rather than having to clone the expr to special case the root, could you instead check the root initially?

Something like

if let Expr::Unnest(Unnest { expr })  = original_expr {
  let expr = expr.transform_up(...);
  Ok(transform(transformex_expe)
}

And similarly for the others?

[duongcongtoai]

I tried that approach, but it will add more logic to the code, something like:

    if let Expr::Unnest(Unnest { expr: ref arg }) = original_expr {
        let Transformed {
            data: transformed_expr,
            transformed,
            tnr: _,
        } = arg.clone().transform_up(|expr: Expr| {
...
        })?;
        // no transformation eligible in the children
        // only then the root unnest can be transformed
        if !transformed {
            return transform(&transformed_expr, arg);
        } else {
            return Ok(vec![transformed_expr]);
        }
    }

    // from this onward, unnest(struct) is invalid because
    // we are sure that root_expr is not unnest expr
    let Transformed {
        data: transformed_expr,
        transformed,
        tnr: _,
    } = original_expr.clone().transform_up(|expr: Expr| {
...
    if !transformed {
        if matches!(&transformed_expr, Expr::Column(_)) {
            inner_projection_exprs.push(transformed_expr.clone());
            Ok(vec![transformed_expr])
        } else {
            // We need to evaluate the expr in the inner projection,
            // outer projection just select its name
            let column_name = transformed_expr.display_name()?;
            inner_projection_exprs.push(transformed_expr);
            Ok(vec![Expr::Column(Column::from_name(column_name))])
        }
    } else {
        Ok(vec![transformed_expr])
    }

    })?;

For the clone() part, i don't know how to avoid it because transform_up will take the ownership of the expr
Please let me know your opinion

[alamb]

What you have is good -- thank you for trying this

[alamb]

could you also please add tests for:

1. Multiple unnest: select unnest(col1), unnest(col1), select unnest(col1), unnest(col12)
2. Unnest + expression: select unnest(col1)+1
3. Unnest on a non structured column (e.g. unnest('foo'))
4. Triple unnest unnest(unnest(unnnest(..)))

(it is ok if they error, but I think it would be useful to have the additional test coverage that demonstrates how sophisticated the implementation is)

[duongcongtoai]

i added more tests

[alamb]

Thank you @duongcongtoai

[jayzhan211]

Thanks @duongcongtoai and @alamb