Skip to content

Minor: clean up TODO comments in unnest.slt#12795

Merged
alamb merged 1 commit intoapache:mainfrom
goldmedal:chore/clean-up-todo-unnest
Oct 8, 2024
Merged

Minor: clean up TODO comments in unnest.slt#12795
alamb merged 1 commit intoapache:mainfrom
goldmedal:chore/clean-up-todo-unnest

Conversation

@goldmedal
Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Remove some legacy TODO comments and refine the tests for unnest.

Are these changes tested?

the original CI

Are there any user-facing changes?

no

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Oct 7, 2024
5
6

## FIXME: https://github.com/apache/datafusion/issues/11198
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue has been closed. The error assertion makes sense to me. I also added another positive test at L539


## TODO: support unnest as a child expr
query error DataFusion error: Internal error: unnest on struct can only be applied at the root level of select expression
select arrow_typeof(unnest(column5)) from unnest_table;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error isn't a child expression issue. It's an issue about applying a function to an unnest struct expression. The error message makes sense to me. DuckDB has similar behavior:

D CREATE TABLE unnest_table
  AS VALUES
      ([1,2,3], [7], 1, [13, 14], {'c1': 1, 'c2': 2}),
      ([4,5], [8,9,10], 2, [15, 16], {'c1':3,'c2':4}),
      ([6], [11,12], 3, null, null),
      ([12], [null, 42, null], null, null, {'c1':7,'c2':8}),
      -- null array to verify the `preserve_nulls` option
      (null, null, 4, [17, 18], null)
  ;
D select typeof(unnest(col4)) from unnest_table;
Binder Error: UNNEST() on a struct column can only be applied as the root element of a SELECT expression
LINE 1: select typeof(unnest(col4)) from unnest_table

Comment on lines +517 to +520
query T
select arrow_typeof(unnest(column1)) from unnest_table;
----
Int64
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnest as a child expression works fine if the input isn't a struct.


### TODO: group by unnest struct
query error DataFusion error: Error during planning: Projection references non\-aggregate values
select unnest(column1) c1 from nested_unnest_table group by c1.c0;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the error message is weird, asserting that this query fails makes sense to me.
DuckDB doesn't allow this SQL, too. I can't find other databases with this behavior. 🤔

D CREATE TABLE unnest_table
  AS VALUES
      ([1,2,3], [7], 1, [13, 14], {'c1': 1, 'c2': 2}),
      ([4,5], [8,9,10], 2, [15, 16], {'c1':3,'c2':4}),
      ([6], [11,12], 3, null, null),
      ([12], [null, 42, null], null, null, {'c1':7,'c2':8}),
      -- null array to verify the `preserve_nulls` option
      (null, null, 4, [17, 18], null)
  ;
D select unnest(col4) u1 from unnest_table group by u1.c1;
Binder Error: Referenced table "u1" not found!
Candidate tables: "unnest_table"
LINE 1: ...st(col4) u1 from unnest_table group by u1.c1;

@goldmedal goldmedal marked this pull request as ready for review October 7, 2024 17:10
Comment on lines +801 to +803
# TODO: this query should work. see issue: https://github.com/apache/datafusion/issues/12794
query error DataFusion error: Internal error: unnest on struct can only be applied at the root level of select expression
select unnest(column1) c1 from nested_unnest_table
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #12794 for it.

Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @goldmedal

@alamb alamb merged commit 6d61503 into apache:main Oct 8, 2024
@alamb
Copy link
Copy Markdown
Contributor

alamb commented Oct 8, 2024

🚀

@goldmedal goldmedal deleted the chore/clean-up-todo-unnest branch October 9, 2024 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants