Replace jsqlparser with antlr gramma by akrambek · Pull Request #1298 · aklivity/zilla

akrambek · 2024-10-15T23:45:23Z

jfallows · 2024-10-16T15:56:51Z

...ding-pgsql/src/main/antlr4/io/aklivity/zilla/runtime/binding/pgsql/parser/PostgreSQLLexer.g4

In general we tend to use mixed case even for acronyms in class names, so PostgreSqlLexer.g4 not PostgreSQLLexer.g4 etc.

jfallows · 2024-10-16T15:57:16Z

.../main/java/io/aklivity/zilla/runtime/binding/pgsql/parser/LexerDispatchingErrorListener.java

+    public void reportAmbiguity(Parser recognizer,
+                                DFA dfa,
+                                int startIndex,
+                                int stopIndex,
+                                boolean exact,
+                                BitSet ambigAlts,
+                                ATNConfigSet configs)


Formatting, also other methods.

jfallows · 2024-10-16T15:57:56Z

...main/java/io/aklivity/zilla/runtime/binding/pgsql/parser/ParserDispatchingErrorListener.java

+public class ParserDispatchingErrorListener implements ANTLRErrorListener
+{
+    @SuppressWarnings("checkstyle:MemberName")
+    Parser _parent;


What's going on here with the non-standard field naming?

jfallows · 2024-10-16T15:58:22Z

...main/java/io/aklivity/zilla/runtime/binding/pgsql/parser/ParserDispatchingErrorListener.java

+        String msg,
+        RecognitionException e)
+    {
+        var foo = new ProxyErrorListener(_parent.getErrorListeners());


jfallows · 2024-10-16T15:59:14Z

.../binding-pgsql/src/main/java/io/aklivity/zilla/runtime/binding/pgsql/parser/PgsqlParser.java

+    public SQLTableCommandListener.TableInfo parseTable(
+        String sql)
+    {
+        CharStream input = CharStreams.fromString(sql);
+        lexer.reset();
+        lexer.setInputStream(input);
+
+        tokens.setTokenSource(lexer);
+        parser.setTokenStream(tokens);
+
+        walker.walk(tableCommand, parser.root());
+
+        return tableCommand.tableInfo();
+    }


jfallows · 2024-10-16T16:00:35Z

...pgsql/src/main/java/io/aklivity/zilla/runtime/binding/pgsql/parser/PostgreSQLParserBase.java

+import org.antlr.v4.runtime.TokenStream;
+
+@SuppressWarnings({"all", "warnings", "unchecked", "unused", "cast", "CheckReturnValue"})
+public abstract class PostgreSQLParserBase extends Parser


Suggested change

public abstract class PostgreSQLParserBase extends Parser

public abstract class PostgreSqlParserBase extends Parser

jfallows · 2024-10-16T16:16:33Z

...in/java/io/aklivity/zilla/runtime/binding/pgsql/parser/listener/SQLTableCommandListener.java

+        String tableName,
+        Map<String, String> columns,
+        Set<String> primaryKeys
+    )


This close paren should be on previous line, like methods.

jfallows · 2024-10-18T20:03:25Z

cloud/docker-image/src/main/docker/assembly.xml

        <include>org/junit/**</include>
        <include>com/google/**</include>
        <include>org/checkerframework/**</include>
-        <include>com/github/jsqlparser/**</include>


jfallows · 2024-10-18T20:05:07Z

incubator/binding-pgsql-kafka/pom.xml

-      <artifactId>jsqlparser</artifactId>
+      <groupId>io.aklivity.zilla</groupId>
+      <artifactId>binding-pgsql</artifactId>
+      <scope>provided</scope>


This provided scope should be removed - similar to openapi dependency in openapi-asyncapi binding.

jfallows · 2024-10-18T20:07:35Z

...-pgsql/src/main/java/io/aklivity/zilla/runtime/binding/pgsql/parser/PostgreSqlLexerBase.java

+import org.antlr.v4.runtime.CharStream;
+import org.antlr.v4.runtime.Lexer;
+
+public abstract class PostgreSqlLexerBase extends Lexer


Is this generated? If so then we shouldn't need to check it in.
Same for any other generated code.

No it is not auto generated the gramma is written around that

https://github.com/antlr/grammars-v4/tree/master/sql/postgresql/Java

jfallows · 2024-10-18T20:09:06Z

...in/java/io/aklivity/zilla/runtime/binding/pgsql/parser/listener/SqlCreateStreamListener.java

+
+public class SqlCreateStreamListener extends PostgreSqlParserBaseListener
+{
+    private String name;


Move the non-final fields below the final fields, makes it easier to read the intent of the code.

jfallows · 2024-10-18T20:10:04Z

...-pgsql/src/main/java/io/aklivity/zilla/runtime/binding/pgsql/parser/module/FunctionInfo.java

+ * WARRANTIES OF ANY KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package io.aklivity.zilla.runtime.binding.pgsql.parser.module;


Why module as the subpackage name?

Sorry I didn't understand the question

jfallows · 2024-10-18T20:10:21Z

...ding-pgsql/src/main/java/io/aklivity/zilla/runtime/binding/pgsql/parser/module/ViewInfo.java

+        String name,
+        String select)


Suggested change

String name,

String select)

String name,

String select)

jfallows · 2024-10-18T20:10:49Z

incubator/binding-pgsql/src/main/moditect/module-info.java

+
+    exports io.aklivity.zilla.runtime.binding.pgsql.parser;
+    exports io.aklivity.zilla.runtime.binding.pgsql.parser.listener;
+    exports io.aklivity.zilla.runtime.binding.pgsql.parser.module;


Suggested change

exports io.aklivity.zilla.runtime.binding.pgsql.parser.module;

exports io.aklivity.zilla.runtime.binding.pgsql.parser.model;

Perhaps you meant model?

yeah thanks!

jfallows · 2024-10-18T20:11:43Z

.../aklivity/zilla/specs/binding/risingwave/streams/effective/create.function.python/client.rpt

                                .build()
                              .build()}
-write "CREATE FUNCTION gcd(int , int)\n"
+write "CREATE FUNCTION gcd(int, int)\n"


Was this changed because it didn't work with the space?
If so, then is the grammar perhaps too strict?

jfallows · 2024-10-18T20:12:33Z

...ty/zilla/specs/binding/risingwave/streams/effective/create.function.return.struct/server.rpt

 read "CREATE FUNCTION key_value(bytea)\n"
-     "RETURNS struct < key varchar , value varchar >\n"
-     "AS key_value\n"
+     "RETURNS struct<key varchar, value varchar>\n"


Same question here about spaces? Checking if they are no longer permitted or not.

akrambek added 3 commits October 15, 2024 16:44

Initial implementation

57984ca

WIP

66622a5

Fix typo

1b0c5ab

jfallows requested changes Oct 16, 2024

View reviewed changes

akrambek added 14 commits October 16, 2024 09:56

Refactor code

a99a47b

Refactor

c158553

WIP

5393150

WIP

5cf9acd

WIP

67aacf6

Revert back change

449e63a

WIP

526fc5a

WIP

3a9a483

WIP

9af3f60

WIP

4704dda

WIP

6ba3bce

WIP only create function left

ae8264e

WIP

982a955

Checkpoint

5d4414d

akrambek marked this pull request as ready for review October 18, 2024 18:02

akrambek added 5 commits October 18, 2024 11:07

Remove regenerated files

e28d843

Clean up

e578609

update module-info.xml

af43caf

update module-info

89787ef

Fix parsing

e344a81

jfallows requested changes Oct 18, 2024

View reviewed changes

akrambek added 4 commits October 18, 2024 15:43

Fix remaining issues

8a89a81

Applied feedback from PR

521343a

avoid object creation

47c5854

Remove comments

0710b08

jfallows approved these changes Oct 22, 2024

View reviewed changes

jfallows merged commit 9e34ab8 into aklivity:develop Oct 22, 2024

	public abstract class PostgreSQLParserBase extends Parser
	public abstract class PostgreSqlParserBase extends Parser

	exports io.aklivity.zilla.runtime.binding.pgsql.parser.module;
	exports io.aklivity.zilla.runtime.binding.pgsql.parser.model;

Conversation

akrambek commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

akrambek commented Oct 15, 2024 •

edited

Loading