Support PostgreSQL array subquery constructor #566

MazterQyou · 2022-08-10T00:54:43Z

This PR adds support for PostgreSQL array subquery constructor (e.g. SELECT ARRAY(SELECT 1 UNION SELECT 2)), as well as a related test.

coveralls · 2022-08-10T01:00:38Z

Pull Request Test Coverage Report for Build 2848516561

62 of 63 (98.41%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 85.444%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser.rs	8	9	88.89%

Totals
Change from base Build 2842935516:	0.07%
Covered Lines:	9498
Relevant Lines:	11116

💛 - Coveralls

MazterQyou · 2022-08-10T01:06:36Z

tests/sqlparser_common.rs

@@ -2518,7 +2518,7 @@ fn parse_bad_constraint() {

 #[test]
 fn parse_scalar_function_in_projection() {
-    let names = vec!["sqrt", "array", "foo"];
+    let names = vec!["sqrt", "foo"];


This may be a controversial change for this test. PostgreSQL doesn't have a function-based array constructor (array(1, 2, 3)), nor can I find information on other databases using such a syntax. Some, like BigQuery, use the ARRAY(subquery) syntax implemented there. It'd be great to hear some thoughts on this.

This test was added in #432

Can you review the description on that PR and let me know what you think?

Interestingly, postgres doesn't allow the syntax used in the DataFusion test (and referred to in #432)

alamb=# SELECT array(c1, cast(c2 as varchar)) FROM test; ERROR: syntax error at or near "c1" LINE 1: SELECT array(c1, cast(c2 as varchar)) FROM test; ^

https://github.com/apache/arrow-datafusion/blame/834924f274d9e04444030b8eb3e07e1035fcd3cf/datafusion/core/tests/sql/functions.rs#L134 doesn't really help me figure out from who the test originated from as it was moved (maybe some more poking around could find the original author)

@ovr do you remember any of the previous history here?

🤔

I poked around and found this commit: apache/datafusion@98b710a

I poked around and found this commit: apache/datafusion@98b710a

Nice -- that came in from apache/arrow#8102 / https://issues.apache.org/jira/browse/ARROW-9902 by @jorgecarleitao and approved by @andygrove

Can either if you remember if/how using array as a function (rather than the more specialized SQL support offered by postgres) was important? We are triyng to sort out how important backwards compatibility is in this case

I support this as long as DataFusion team's okay with that. I haven't been able to find a dialect with built-in array function, but at least two of them with built-in array subquery constructor.

@alamb do you feel it's a good idea to remove the GenericDialiect | PostgreSqlDialect check now so later someone can make negated checks for dialects that shouldn't go the subquery route?

I haven't been able to find a dialect with built-in array function, but at least two of them with built-in array subquery constructor.

Here is the spark reference docs: https://spark.apache.org/docs/2.4.1/api/sql/index.html#array (though perhaps you mean there is no SparkDialect in this crate)?

do you feel it's a good idea to remove the GenericDialiect | PostgreSqlDialect check now so later someone can make negated checks for dialects that shouldn't go the subquery route?

Sounds reasonable to me -- can you also possibly link one or two examples from other dialects that support the array subquery constructor?

PostgreSQL array subquery constructor (in fact, all of array constructors): https://www.postgresql.org/docs/current/sql-expressions.html#SQL-SYNTAX-ARRAY-CONSTRUCTORS

I haven't been able to find info on other dialects present in the crate to support array subquery, but at least CockroachDB and BigQuery support array subquery.

However while searching around I found that ClickHouse (which we do have a dialect for) actually has array function constructor built in: https://clickhouse.com/docs/en/sql-reference/functions/array-functions/#arrayx1--operator-x1-

So we can either make a decision for leaving this on for specific dialects (GenericDialect | PostgreSqlDialect for instance) or negate this to support ARRAY(x1, x2, …) for dialects known to support array function constructor (currently ClickHouseDialect only).

Let's negate it for ClickHouseDialect and call it a day

alamb

Thanks @MazterQyou -- the code looks great but as you have identified I am not sure about the array test change. I left some comments

alamb · 2022-08-11T11:05:41Z

tests/sqlparser_common.rs

@@ -2518,7 +2518,7 @@ fn parse_bad_constraint() {

 #[test]
 fn parse_scalar_function_in_projection() {
-    let names = vec!["sqrt", "array", "foo"];
+    let names = vec!["sqrt", "foo"];


This test was added in #432

Can you review the description on that PR and let me know what you think?

alamb · 2022-08-11T11:09:49Z

tests/sqlparser_common.rs

@@ -2518,7 +2518,7 @@ fn parse_bad_constraint() {

 #[test]
 fn parse_scalar_function_in_projection() {
-    let names = vec!["sqrt", "array", "foo"];
+    let names = vec!["sqrt", "foo"];


Interestingly, postgres doesn't allow the syntax used in the DataFusion test (and referred to in #432)

alamb=# SELECT array(c1, cast(c2 as varchar)) FROM test; ERROR: syntax error at or near "c1" LINE 1: SELECT array(c1, cast(c2 as varchar)) FROM test; ^

https://github.com/apache/arrow-datafusion/blame/834924f274d9e04444030b8eb3e07e1035fcd3cf/datafusion/core/tests/sql/functions.rs#L134 doesn't really help me figure out from who the test originated from as it was moved (maybe some more poking around could find the original author)

@ovr do you remember any of the previous history here?

🤔

MazterQyou · 2022-08-12T18:08:06Z

@alamb updated the if clause with dialects and added a test for ClickHouse's array function (renaming the file in the process due to a typo 😶). Please re-review

alamb

Looks great -- thanks for sticking with it @MazterQyou

alamb · 2022-08-12T18:08:23Z

tests/sqlparser_clickhouse.rs

@@ -121,6 +121,25 @@ fn parse_array_expr() {
    )
 }

+#[test]


👌 Very nice @MazterQyou

MazterQyou force-pushed the upstream-patch/array-subquery branch from c6edf5f to 7ed261e Compare August 10, 2022 00:58

MazterQyou commented Aug 10, 2022

View reviewed changes

alamb reviewed Aug 11, 2022

View reviewed changes

alamb mentioned this pull request Aug 12, 2022

Remove support for array function and only support array[] style postgres syntax apache/datafusion#3115

Closed

alamb approved these changes Aug 12, 2022

View reviewed changes

MazterQyou force-pushed the upstream-patch/array-subquery branch 2 times, most recently from 13ce6f5 to 01a69b2 Compare August 12, 2022 18:00

Support PostgreSQL array subquery constructor

76e5030

MazterQyou force-pushed the upstream-patch/array-subquery branch from 01a69b2 to 76e5030 Compare August 12, 2022 18:04

MazterQyou requested a review from alamb August 12, 2022 18:08

alamb reviewed Aug 12, 2022

View reviewed changes

tests/sqlparser_clickhouse.rs

@@ -121,6 +121,25 @@ fn parse_array_expr() {

)

}

#[test]

Copy link

Collaborator

alamb Aug 12, 2022

Choose a reason for hiding this comment

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

👌 Very nice @MazterQyou

alamb approved these changes Aug 12, 2022

View reviewed changes

alamb merged commit 31ba001 into sqlparser-rs:main Aug 12, 2022

MazterQyou deleted the upstream-patch/array-subquery branch August 12, 2022 18:10

alamb mentioned this pull request Aug 13, 2022

'Rename array() function to make_array(), extend array[] apache/datafusion#3122

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support PostgreSQL array subquery constructor #566

Support PostgreSQL array subquery constructor #566

MazterQyou commented Aug 10, 2022

coveralls commented Aug 10, 2022 •

edited

MazterQyou Aug 10, 2022

alamb Aug 11, 2022

alamb Aug 11, 2022

MazterQyou Aug 11, 2022

alamb Aug 11, 2022 •

edited

MazterQyou Aug 12, 2022

MazterQyou Aug 12, 2022 •

edited

alamb Aug 12, 2022

MazterQyou Aug 12, 2022

alamb Aug 12, 2022

alamb left a comment

alamb Aug 11, 2022

alamb Aug 11, 2022

MazterQyou commented Aug 12, 2022

alamb left a comment

alamb Aug 12, 2022

@@ @@ -121,6 +121,25 @@ fn parse_array_expr() { @@
                   )
               }
+              #[test]

Support PostgreSQL array subquery constructor #566

Support PostgreSQL array subquery constructor #566

Conversation

MazterQyou commented Aug 10, 2022

coveralls commented Aug 10, 2022 • edited

Pull Request Test Coverage Report for Build 2848516561

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Aug 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MazterQyou Aug 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MazterQyou commented Aug 12, 2022

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 10, 2022 •

edited

alamb Aug 11, 2022 •

edited

MazterQyou Aug 12, 2022 •

edited