-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add support for TableFunction [WITH OFFSET|ORDINALITY] #2219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This PR fixes JSQLParser#2218, by adding a new `withClause` to a TableFunction, which allows for support of SQL-99's `UNNEST(...) WITH ORDINALITY` or GoogleSQL's `UNNEST(...) WITH OFFSET`. Currently, this is a feeler PR, as I'm getting a test failure, as the output of the parsing differs in my tests: ``` org.opentest4j.AssertionFailedError: Output from Deparser does not match. ==> Expected :select*from unnest(array[1,2,3])with offset as t(a,b) Actual :select*from unnest(array[1,2,3])as t(a,b) <Click to see difference> at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151) at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132) at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197) at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182) at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1156) at [email protected]/net.sf.jsqlparser.test.TestUtils.assertStatementCanBeDeparsedAs(TestUtils.java:126) at [email protected]/net.sf.jsqlparser.test.TestUtils.assertSqlCanBeParsedAndDeparsed(TestUtils.java:99) at [email protected]/net.sf.jsqlparser.test.TestUtils.assertSqlCanBeParsedAndDeparsed(TestUtils.java:85) at [email protected]/net.sf.jsqlparser.statement.select.TableFunctionTest.testTableFunctionWithSupportedWithClauses(TableFunctionTest.java:60) at java.base/java.lang.reflect.Method.invoke(Method.java:580) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:1024) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596) at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:276) at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596) at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:276) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596) at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:276) at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596) ```
…ithClause where present
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff and thank you for your work and contribution.
Please change the token definition and then I will merge.
@@ -545,6 +545,8 @@ TOKEN: /* SQL Keywords. prefixed with K_ to avoid name clashes */ | |||
| <K_WHERE:"WHERE"> | |||
| <K_WINDOW:"WINDOW"> | |||
| <K_WITH:"WITH"> | |||
| <K_WITH_OFFSET:"WITH OFFSET"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the compound token. Just WITH
and ORDINALITY
/ The goal must be to define as few tokens as possible.
} | ||
{ | ||
[ prefix = <K_LATERAL> ] | ||
function=Function() | ||
[ withClause = <K_WITH_OFFSET> | withClause = <K_WITH_ORDINALITY> ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ <K_WITH> ( withClause = <K_OFFSET> | withClause = <K_ORDINALITY> ) ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried changing to this, but there is a new test failing:
KeywordsTest.testRelObjectNameWithoutValue(String)
Keyword ORDINALITY
net.sf.jsqlparser.JSQLParserException: net.sf.jsqlparser.parser.ParseException: Encountered unexpected token: "SELECT" <K_SELECT>
at line 1, column 1.
Was expecting one of:
"WITH"
at [email protected]/net.sf.jsqlparser.parser.CCJSqlParserUtil.parseStatement(CCJSqlParserUtil.java:352)
at [email protected]/net.sf.jsqlparser.parser.CCJSqlParserUtil.parse(CCJSqlParserUtil.java:125)
at [email protected]/net.sf.jsqlparser.parser.CCJSqlParserUtil.parse(CCJSqlParserUtil.java:91)
at [email protected]/net.sf.jsqlparser.test.TestUtils.assertSqlCanBeParsedAndDeparsed(TestUtils.java:98)
at [email protected]/net.sf.jsqlparser.test.TestUtils.assertSqlCanBeParsedAndDeparsed(TestUtils.java:85)
at [email protected]/net.sf.jsqlparser.statement.KeywordsTest.testRelObjectNameWithoutValue(KeywordsTest.java:53)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the issue, I needed to add ORDINALITY to the restricted SQL2016 keywords
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried changing to this, but there is a new test failing:
KeywordsTest.testRelObjectNameWithoutValue(String)
Please see here for an explanation: https://manticore-projects.com/JSQLParser/contribution.html#manage-reserved-keywords
I don't think, we need to reserve those keywords but only whitelist them properly by running gradle updateKeywords
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made those changes, and pushed up. Thank you
@@ -47,4 +49,14 @@ void testTableFunctionWithNamedParameterWhereNameIsOuterKeyword() throws JSQLPar | |||
" lateral flatten(input => i.DATA_VALUE:Friends, outer => true) AS f1;"; | |||
TestUtils.assertSqlCanBeParsedAndDeparsed(sqlStr, true); | |||
} | |||
|
|||
@ParameterizedTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
…s, adding ORDINALITY keyword.
Ready for re-review. |
…Utils, and updating pushing the result of `gradle updateKeywords`
Thank you a lot for your work and contribution. Merged. |
This PR fixes #2218, by adding a new
withClause
to a TableFunction, which allows for support of SQL-99'sUNNEST(...) WITH ORDINALITY
or GoogleSQL'sUNNEST(...) WITH OFFSET
.