Add parser support for SQL MERGE by djsagain · Pull Request #7126 · trinodb/trino

djsagain · 2021-03-02T19:39:14Z

Since complete support for SQL MERGE in Trino and the Hive connector is 5,700 lines of code, @electrum suggested that the parser changes be submitted as this separate PR.

This single commit adds the syntax rules and tree classes needed to parse and represent SQL MERGE syntax, accompanied by parser tests.

electrum

Looks good overall

core/trino-parser/src/main/java/io/trino/sql/tree/Merge.java

core/trino-parser/src/main/java/io/trino/sql/tree/MergeDelete.java

core/trino-parser/src/main/java/io/trino/sql/tree/MergeInsert.java

core/trino-parser/src/main/java/io/trino/sql/tree/MergeUpdate.java

core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java

djsagain · 2021-03-03T04:40:12Z

Thanks for the very prompt review, @electrum! I've force-pushed the suggested changes.

kasiafi

A couple of comments and questions. This is a great change!

core/trino-parser/src/main/java/io/trino/sql/tree/MergeInsert.java

core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java

core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4

core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java

core/trino-parser/src/main/java/io/trino/sql/tree/Join.java

core/trino-parser/src/main/java/io/trino/sql/tree/MergeCase.java

core/trino-parser/src/main/java/io/trino/sql/tree/AstVisitor.java

core/trino-parser/src/main/java/io/trino/sql/tree/DefaultTraversalVisitor.java

kasiafi · 2021-03-03T12:45:17Z

As part of this PR, please catch MERGE statements in the StatementAnalyzer and fail them with NOT_SUPPORTED.

djsagain · 2021-03-03T18:52:04Z

Thanks for the excellent, detailed review, @kasiafi! I force-pushed changes that address your review comments.

docs/src/main/sphinx/sql/merge.rst

kasiafi · 2021-03-04T12:12:47Z

Thanks for updating!
Please address this comment: #7126 (comment)

I can see there's a bunch of tests failing as a result of MERGE being a reserved word. Actually, it collides with a function name merge. I think, it should be non-reserved then. But it is a reserved keyword. @martint what's your opinion?

@mosabua could you please review the docs part?

martint · 2021-03-04T15:31:03Z

I can see there's a bunch of tests failing as a result of MERGE being a reserved word. Actually, it collides with a function name merge. I think, it should be non-reserved then. But it is a reserved keyword. @martint what's your opinion?

If they appear in non-conflicting contexts, we can make it a non-reserved keyword. Otherwise, we'll have to figure out how to deprecate and rename the conflicting function.

This commit adds the syntax rules and tree classes needed to parse and represent SQL MERGE syntax, accompanied by parser tests. Trino engine and Hive connector changes that implement SQL MERGE will be added in a follow-on PR.

djsagain · 2021-03-04T19:22:48Z

If they appear in non-conflicting contexts, we can make it a non-reserved keyword. Otherwise, we'll have to figure out how to deprecate and rename the conflicting function.

I was mistaken - - the SQL 2016 standard that introduced the MERGE statement does not define the token "MERGE" as a reserved word. I removed the reserved word status for MERGE in this PR, and the merge(HyperLogLog) parses again. Sorry for the misconception.

djsagain · 2021-03-04T19:24:56Z

As part of this PR, please catch MERGE statements in the StatementAnalyzer and fail them with NOT_SUPPORTED.

Done.

mosabua

See my comment about Limitations for connectors.

mosabua · 2021-03-09T21:42:34Z

docs/src/main/sphinx/sql/merge.rst

+Limitations
+-----------
+
+Some connectors have limited or no support for ``MERGE``.


This is similar to what is in UPDATE .. I am concerned that this is not really true at this stage. Its more like "Nearly no connectors have support for MERGE.". And the connector docs also dont have this in a limitations section consistently.

@electrum I think we should ensure all connectors have a Limitations section and then add MERGE and UPDATE to all the ones affected.

I feel like we need a better way to address this than making it an NxM problem.

Agreed .. the best would be consistent implementation of such things across all connectors so we just have to call out a few exceptions. Currently I think it would be great to just go in depth on the docs and then use that assembled knowledge to drive consistency in the implementation to clean things up. Currently its all kind of hidden in the code only and thats just really hard for users (and probably even developers when assessing impact of changes..)

But all this should be a separate larger effort outside of the scope of this PR for sure..

ANTLR syntax rules, tree classes to parse and represent SQL MERGE syntax, and parser tests. Changes were done after reviewing the following Trino PR: trinodb/trino#7126 So, this commit is deeply inspired by Trino's implementation. Co-authored-by: David Stryker <david.stryker@starburstdata.com>

cla-bot bot added the cla-signed label Mar 2, 2021

djsagain requested review from electrum, kasiafi and martint March 2, 2021 19:39

electrum approved these changes Mar 2, 2021

View reviewed changes

djsagain force-pushed the david.stryker/sql-merge-parser branch from 5e39f14 to 7d36957 Compare March 3, 2021 04:38

kasiafi reviewed Mar 3, 2021

View reviewed changes

djsagain force-pushed the david.stryker/sql-merge-parser branch from 7d36957 to 422afe6 Compare March 3, 2021 18:48

djsagain force-pushed the david.stryker/sql-merge-parser branch from 422afe6 to 904b6a7 Compare March 4, 2021 03:57

kasiafi reviewed Mar 4, 2021

View reviewed changes

docs/src/main/sphinx/sql/merge.rst Outdated Show resolved Hide resolved

djsagain force-pushed the david.stryker/sql-merge-parser branch from 904b6a7 to 3c06db2 Compare March 4, 2021 15:27

djsagain force-pushed the david.stryker/sql-merge-parser branch from 3c06db2 to 9fb94d0 Compare March 4, 2021 18:55

David Stryker added 2 commits March 4, 2021 11:05

Add parser support for SQL MERGE

e482270

This commit adds the syntax rules and tree classes needed to parse and represent SQL MERGE syntax, accompanied by parser tests. Trino engine and Hive connector changes that implement SQL MERGE will be added in a follow-on PR.

Add parser test case for SearchedCaseExpressions

3d80207

djsagain force-pushed the david.stryker/sql-merge-parser branch from 9fb94d0 to c63ac00 Compare March 4, 2021 19:18

Remove unused method AstBuilder.getTextIfPresent()

af99147

djsagain force-pushed the david.stryker/sql-merge-parser branch from c63ac00 to af99147 Compare March 9, 2021 18:38

mosabua requested changes Mar 9, 2021

View reviewed changes

mosabua approved these changes Mar 12, 2021

View reviewed changes

electrum merged commit 832c0de into trinodb:master Mar 12, 2021

djsagain deleted the david.stryker/sql-merge-parser branch March 13, 2021 14:42

findepi mentioned this pull request Apr 22, 2021

MERGE statement #7708

Closed

6 tasks

acarpente-denodo mentioned this pull request Jul 1, 2025

feat(plugin-iceberg): Add support for MERGE INTO prestodb/presto#25470

Merged

6 tasks

acarpente-denodo mentioned this pull request Oct 10, 2025

feat: Add SQL Support for MERGE INTO in Presto (engine) prestodb/presto#26278

Merged

6 tasks

acarpente-denodo mentioned this pull request Oct 21, 2025

feat: Add SQL Support for MERGE INTO in Presto (engine) [sourcery-ai] prestodb/presto#26376

Closed

6 tasks

acarpente-denodo mentioned this pull request Nov 18, 2025

feat: Add SQL Support for MERGE INTO in Presto (efficient workload distribution) acarpente-denodo/presto#1

Open

6 tasks

Conversation

djsagain commented Mar 2, 2021

Uh oh!

electrum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djsagain commented Mar 3, 2021

Uh oh!

kasiafi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kasiafi commented Mar 3, 2021

Uh oh!

djsagain commented Mar 3, 2021

Uh oh!

Uh oh!

kasiafi commented Mar 4, 2021

Uh oh!

martint commented Mar 4, 2021

Uh oh!

djsagain commented Mar 4, 2021

Uh oh!

djsagain commented Mar 4, 2021

Uh oh!

mosabua left a comment

Choose a reason for hiding this comment

Uh oh!

mosabua Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

electrum Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

mosabua Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

mosabua Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants