Support standard UNION/INTERSECT syntax #248

dinodork · 2024-08-30T10:10:19Z

The SQL standard allows parentheses around all query expressions (SELECTs) in a UNION. The Bison parser cannot currently handle queries like (SELECT 1) UNION (SELECT 1), let alone ((SELECT 1) UNION (SELECT 1)).

dey4ss · 2024-09-03T06:28:02Z

Just approved to run the actions, I will review your PR in the next week. I noticed that the gcc-6 stage fails because manually cloning does not work with forked repositories, I will see if I can provide a fix for that.

dinodork · 2024-09-03T08:01:54Z

Just approved to run the actions, I will review your PR in the next week. I noticed that the gcc-6 stage fails because manually cloning does not work with forked repositories, I will see if I can provide a fix for that.

Thanks a lot. Since you're on it, I removed to last commit where I was trying to fix that issue, not doing a very good job at it.

dey4ss · 2024-09-12T06:04:45Z

The CI stage should work now if you merge the lastest update to master.

dinodork · 2024-09-12T08:34:42Z

The CI stage should work now if you merge the lastest update to master.

Thanks. All green now!

dey4ss

I just did a quick pass with some easy-to-fix things, e.g., code style. I will need some more time for an in-depth review because the changes are quite heavy. I appreciate that you kept the naming and grammar closer to the SQL standard, which should make it easier to review.

dey4ss · 2024-09-16T09:13:05Z

Makefile

@@ -70,7 +70,7 @@ $(SRCPARSER)/flex_lexer.o: $(SRCPARSER)/flex_lexer.cpp $(SRCPARSER)/bison_parser
 	$(CXX) $(LIB_CFLAGS) -c -o $@ $< -Wno-sign-compare -Wno-unneeded-internal-declaration -Wno-register

 %.o: %.cpp $(PARSER_CPP) $(LIB_H)
-	$(CXX) $(LIB_CFLAGS) -c -o $@ $<
+	$(CXX) $(LIB_CFLAGS) $(CXXFLAGS) -c -o $@ $<


We don't use additonal flags in our base repo builds. Unless there's a reason to have them here, I'd ask you to remove them.

TL;DR: CFLAGS/CXXFLAGS is a Makefile convention, for example for a program

https://en.wikipedia.org/wiki/CFLAGS
https://www.gnu.org/software/make/manual/make.html#index-CFLAGS

#include <iostream> int main() { #ifdef FOO std::cout << "Foo\n"; #endif }

and, using make's default rule:

$ CXXFLAGS=-DFOO make -B cflagtest && ./cflagtest g++ -DFOO cflagtest.cpp -o cflagtest Foo

It's not that common to have hand-written Makefiles that call other hand-written Makefiles - you normally let a build system such as CMake or meson do that for you - but when you do, you need to make to sure you follow this convention and pass along CFLAGS/CXXFLAGS. You can of course choose your own names for them if you want, but that would be confusing IMHO.

dey4ss · 2024-09-16T09:13:12Z

Makefile

@@ -137,7 +137,7 @@ test: $(TEST_BUILD)

 $(TEST_BUILD): $(TEST_ALL) $(LIB_BUILD)
 	@mkdir -p $(BIN)/
-	$(CXX) $(TEST_CFLAGS) $(TEST_CPP) -o $(TEST_BUILD) -lsqlparser -lstdc++
+	$(CXX) $(CXXFLAGS) $(TEST_CFLAGS) $(TEST_CPP) -o $(TEST_BUILD) -lsqlparser -lstdc++


(see above)

dey4ss · 2024-09-16T09:13:26Z

src/parser/bison_parser.y

@@ -63,6 +63,8 @@
 // %output  "bison_parser.cpp"
 // %defines "bison_parser.h"

+%expect 0


What does this do?

https://www.gnu.org/software/bison/manual/html_node/Expect-Decl.html

In other words, bails out with an error if there's a shift/reduce conflict.

dey4ss · 2024-09-16T09:15:24Z

src/parser/bison_parser.y

-  $$->setOperations->back()->resultLimit = $5;
-  $$->lockings = $6;
-};
+select_statement:


pick here: The indentation does not match the other code. Could ypu please fix that (e.g., by calling the format.sh script)?

Btw, the formatting script has a bug where it places a space after % in a %prec declaration (see last commit)

dey4ss · 2024-09-16T09:17:49Z

src/parser/bison_parser.y

+            $$ = $2;
+          }
+
+/*


knit-picky: We either use C-style comments with an asterisk * on each line or C++-style single line comments.

*nit-pick ;)

Done.

dey4ss · 2024-09-16T09:20:07Z

src/parser/bison_parser.y

+query_term:
+          query_primary
+        ;


knit-picky: we usually put short declarations on a single line. I also see no reason to have a non-terminal that only forwards to another one.

dey4ss · 2024-09-16T09:22:29Z

src/parser/bison_parser.y

+subquery:
+          '(' query_expression ')'
+          {
+            $$ = $2;
+          }
+        ;


We prefer the following style:

Suggested change

subquery:

'(' query_expression ')'

{

$$ = $2;

}

;

subquery: '(' query_expression ')' {

$$ = $2;

};

(or even a one-liner)

dey4ss · 2024-09-16T09:23:11Z

test/queries/queries-good.sql

+((SELECT 1)) UNION ((SELECT 1));
+(((SELECT 1)) UNION ((SELECT 1)));
+((((SELECT 1)) UNION ((SELECT 1))));
+# JOIN# JOIN


Suggested change

# JOIN# JOIN

# JOIN

Oops, fixed.

dey4ss · 2024-09-16T09:25:31Z

test/queries/queries-good.sql

@@ -18,7 +18,35 @@ SELECT * FROM t WHERE a = ? AND b = ?;
 SELECT City.name, Product.category, SUM(price) FROM fact INNER JOIN City ON fact.city_id = City.id INNER JOIN Product ON fact.product_id = Product.id GROUP BY City.name, Product.category;
 SELECT SUBSTR(a, 3, 5) FROM t;
 SELECT * FROM t WHERE a = DATE '1996-12-31';
-# JOIN
+SELECT 1 UNION SELECT 1;


You added a lot of things that I would love to see tested more properly. Could you please add more complex examples with actual subqueries? I would also be interested in some edge cases that are not on the happy path, e.g., in queries-bad.sql. I also encourage you to think of a thorough test case in select_tests.cpp.

Good catch, I actually broke union-in-subquery syntax. I added a new commit that addresses it.

- Renamed most rules starting with select_ to the names used in the SQL standard, <query expression>, <query expression body>, <query term>, <query primary> and <subquery>, respectively. - Added %expect 0 - The previous rule for UNION/INTERSECT syntax was right-recursive, and so the parse tree is right-deep. This patch only changes the rule, you don't want right-recursion in an LALR parser, and moreover it's not standard. It doesn't change the parse tree structure, so we need an extra wrinkle to build the parse tree top-down. - Indentation follows the most common standard, found in MySQL and Postgres. Future work: INTERSECT has higher priority than UNION in the standard, but these rules give them the same priority.

refactoring, we now have to add only a single rule to support them all. Quite surprising, there are not shift/reduce conflicts, which are known to occur in both the MySQL and Postgres parsers for this construction.

syntax that worked. A union without parentheses was actually working. Now we're making the whole thing work. This introduces an ambiguity, though, so we fix that with a %prec declaration.

dinodork · 2024-09-19T14:56:02Z

@dey4ss Conflicts resolved, please feel free to start the CI workflow.

dinodork · 2024-10-07T09:51:31Z

Bump.

dinodork · 2024-10-22T09:01:12Z

Bump #2 @dey4ss

Bouncner · 2024-11-01T12:55:44Z

Hi @dinodork, we currently lack the time to review this PR properly. We would like to merge it, but not without proper reviewing and that might take some more time. Please bear with us.

dinodork force-pushed the query-expression branch from 8448bd1 to 99e7d29 Compare September 3, 2024 08:00

dey4ss mentioned this pull request Sep 11, 2024

Fix Cloning in CI for Forks #249

Merged

dinodork force-pushed the query-expression branch from 99e7d29 to 76d498f Compare September 12, 2024 07:01

dey4ss reviewed Sep 16, 2024

View reviewed changes

dinodork force-pushed the query-expression branch from 76d498f to 3595906 Compare September 16, 2024 15:32

Martin Hansson added 5 commits September 19, 2024 16:39

Added standard support for CXXFLAGS.

27d77a7

All of these queries are valid standard SQL. Thanks to the previous

d5e68fd

refactoring, we now have to add only a single rule to support them all. Quite surprising, there are not shift/reduce conflicts, which are known to occur in both the MySQL and Postgres parsers for this construction.

The previous change actually destroyed the part of subquery

cafbfb4

syntax that worked. A union without parentheses was actually working. Now we're making the whole thing work. This introduces an ambiguity, though, so we fix that with a %prec declaration.

Manual edit needed after running format.sh.

04fe8df

dinodork force-pushed the query-expression branch from 3595906 to 04fe8df Compare September 19, 2024 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support standard UNION/INTERSECT syntax #248

Support standard UNION/INTERSECT syntax #248

dinodork commented Aug 30, 2024

dey4ss commented Sep 3, 2024

dinodork commented Sep 3, 2024

dey4ss commented Sep 12, 2024

dinodork commented Sep 12, 2024

dey4ss left a comment

dey4ss Sep 16, 2024

dinodork Sep 16, 2024 •

edited

Loading

dey4ss Sep 16, 2024

dey4ss Sep 16, 2024

dinodork Sep 16, 2024

dey4ss Sep 16, 2024

dinodork Sep 16, 2024

dinodork Sep 16, 2024

dey4ss Sep 16, 2024

dinodork Sep 16, 2024 •

edited

Loading

dey4ss Sep 16, 2024

dey4ss Sep 16, 2024

dinodork Sep 16, 2024

dey4ss Sep 16, 2024

dinodork Sep 16, 2024

dey4ss Sep 16, 2024 •

edited

Loading

dinodork Sep 16, 2024

dinodork commented Sep 19, 2024 •

edited

Loading

dinodork commented Oct 7, 2024

dinodork commented Oct 22, 2024

Bouncner commented Nov 1, 2024

Support standard UNION/INTERSECT syntax #248

Are you sure you want to change the base?

Support standard UNION/INTERSECT syntax #248

Conversation

dinodork commented Aug 30, 2024

dey4ss commented Sep 3, 2024

dinodork commented Sep 3, 2024

dey4ss commented Sep 12, 2024

dinodork commented Sep 12, 2024

dey4ss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinodork Sep 16, 2024 • edited Loading

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

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

dinodork Sep 16, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dey4ss Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinodork commented Sep 19, 2024 • edited Loading

dinodork commented Oct 7, 2024

dinodork commented Oct 22, 2024

Bouncner commented Nov 1, 2024

dinodork Sep 16, 2024 •

edited

Loading

dinodork Sep 16, 2024 •

edited

Loading

dey4ss Sep 16, 2024 •

edited

Loading

dinodork commented Sep 19, 2024 •

edited

Loading