PHOENIX-7198 support for multi row constructors in single upsert query by richardantal · Pull Request #2222 · apache/phoenix

richardantal · 2025-07-14T14:12:21Z

No description provided.

richardantal · 2025-07-14T14:14:57Z

Probably I need to add more tests for this functionality.
Please review it, I would like to know what do you think about it.

virajjasani · 2025-07-14T22:51:12Z

This could be a nice feature!
I am curious about the use case: Is this required for auto-commit connections from sqlline?

tkhurana · 2025-07-14T23:13:41Z

@richardantal Is there performance gain from using multiple values in a single query ? I don't think so since we can achieve the same by batching multiple upsert statements and then doing a commit.

stoty · 2025-07-15T06:30:23Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/MultipleUpsertIT.java

+        conn.createStatement().execute("UPSERT INTO " + tableName + "(K, INT, INT2) VALUES ('E', 5, 5),('F', 61, 6)");
+        conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('C', 31, 32),('D', 41, 42)");
+        conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('G', 7, 72),('H', 8)");
+        conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('I', 9),('I', 10)");


I don't think this is valid standard SQL (i.e. not all columns are specified, but no column list either) , but if this works for the single value upsert then it's fine.

stoty · 2025-07-15T06:32:40Z

phoenix-core/src/test/java/org/apache/phoenix/parse/QueryParserTest.java

+    }
+
+    @Test
+    public void testValidMultipleUpsert3() throws Exception {


A few more failure cases to exercise the parser would be nice.
i.e. no value list at all, no commas between, not closing some parentheses and similar.

stoty · 2025-07-15T06:34:47Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/MultipleUpsertIT.java

+        conn.createStatement().execute(ddl);
+
+
+        conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('A', 11, 12)");


I'd like to also have some tests for expressions and mixed expressions and literals.
i.e.
VALUES (substring('APPLE',0,1), 2*2)

stoty · 2025-07-15T06:38:16Z

phoenix-core-client/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java

-                                    + constantExpression.toString() + " in column " + column);
+
+            int index = -1;
+            for (byte[][] valuesListIem : valuesList) {


Keeping the two lists in sync like this awkward.
Using an old-style for() would be more readable to me:
for(int index=0; index<valuesList.length();i++)
and then use the index to get both the valuesListItem and constantExpression.

Also a typo in valuesListIem

stoty · 2025-07-15T06:47:48Z

@richardantal Is there performance gain from using multiple values in a single query ? I don't think so since we can achieve the same by batching multiple upsert statements and then doing a commit.

My take:

Avoids network RTT for Query Server
Avoids multiple query parse and compilation phases

also:

Adds an previously unimplemented SQL feature.
Improves compatibility with existing SQL tools that expect thisfeature (though in reality, most of those tools don't handle upserts anyway)

richardantal · 2025-07-16T11:28:35Z

Thanks for the reviews, I added a bit more tests

I also uploaded a simple performance test for this feature to the ticket as an attachment.
For me, inserting simple rows took 65 sec, inserting 2 at once 59, 4: 57, 8: 56 (With the same number of commits)
So there is a minor performance improvement using this feature but it is not the major point of this feature.

stoty · 2025-07-16T11:31:21Z

10% improvement for bulk loading is nothing to sneeze at @richardantal .

stoty

+1 LGTM

stoty · 2025-08-01T04:27:38Z

phoenix-core-client/src/main/antlr3/PhoenixSQL.g

    :   UPSERT (hint=hintClause)? INTO t=from_table_name
        (LPAREN p=upsert_column_refs RPAREN)?
-        ((VALUES LPAREN v=one_or_more_expressions RPAREN ( ON DUPLICATE KEY ( ig=IGNORE |
+        ((VALUES LPAREN e = one_or_more_expressions {v.add(e);} RPAREN (COMMA LPAREN e = one_or_more_expressions {v.add(e);} RPAREN )* ( ON DUPLICATE KEY ( ig=IGNORE |


nit:
Would extracting LPAREN e = one_or_more_expressions {v.add(e);} RPAREN imto a function make sense ?

richardantal · 2025-08-25T12:18:54Z

I rebased the change to resolve conflict with spotless change.
Lets wait for a test run

stoty · 2025-10-09T05:12:55Z

The last CI run still shows spotless issues.
Is there anything blocking this @richardantal ?

richardantal · 2025-10-15T13:08:49Z

I rebase the change
It shouldn't contain spotless issues anymore

virajjasani · 2025-10-15T16:01:09Z

Given the grammar changes, we want this only for 5.4.0 right?

virajjasani · 2025-10-15T16:02:45Z

phoenix-core-client/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java

    }
    boolean isAutoCommit = connection.getAutoCommit();
-    if (valueNodes == null) {
+    if (valueNodesList.isEmpty()) {


Should this not also consider valueNodesList being null? like we consider above with the for-loop?

richardantal · 2025-10-16T08:09:09Z

Thanks Viraj for the question, it is a good one.
In the PhoenixSQL.g I added an init phase for the valueNodesList.
I think it can not be null.
https://github.com/richardantal/phoenix-1/blob/PHOENIX-7198/phoenix-core-client/src/main/antlr3/PhoenixSQL.g#L868

I tweaked the above condition a little, to be closer to what we had earlier.

richardantal · 2025-10-16T08:11:56Z

phoenix-core-client/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java

        targetColumns.add(rowTimestampCol);
-        if (valueNodes != null && !valueNodes.isEmpty()) {
-          valueNodes.add(getNodeForRowTimestampColumn(rowTimestampCol));
+        if (valueNodesList != null) {


Probably this check is not needed, but better to be safe

richardantal · 2026-01-15T07:45:44Z

I think the 2 failing tests (PhoenixTableLevelMetricsIT,MetadataServerConnectionsIT) is unrelated
Also locally they run fine

richardantal · 2026-02-03T16:19:24Z

@virajjasani @kadirozde Can I proceed to merging this?

virajjasani · 2026-02-05T05:26:21Z

It's been long time, sorry i could not take another round of the review. It seemed good in the past. The only concern is, i hope UpsertCompiler does not break any existing UPSERT functionalities.

virajjasani · 2026-02-05T05:27:55Z

Triggered new build: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-2222/8/

@richardantal could you also run spotless:apply if any recent change needs spotless fix?

richardantal requested review from stoty and virajjasani and removed request for stoty July 14, 2025 14:12

virajjasani requested review from haridsv, jpisaac, kadirozde and tkhurana July 14, 2025 22:15

stoty reviewed Jul 15, 2025

View reviewed changes

stoty approved these changes Aug 1, 2025

View reviewed changes

richardantal force-pushed the PHOENIX-7198 branch from 76ce007 to 4844056 Compare August 25, 2025 12:16

richardantal force-pushed the PHOENIX-7198 branch from 4844056 to 9bf2be8 Compare October 15, 2025 13:08

virajjasani reviewed Oct 15, 2025

View reviewed changes

virajjasani requested a review from sanjeet006py October 15, 2025 16:03

richardantal force-pushed the PHOENIX-7198 branch from 9bf2be8 to 8cfb7e3 Compare October 16, 2025 08:02

richardantal commented Oct 16, 2025

View reviewed changes

richardantal force-pushed the PHOENIX-7198 branch from 8cfb7e3 to e2ed715 Compare December 17, 2025 13:57

richardantal force-pushed the PHOENIX-7198 branch from e2ed715 to e20aaf1 Compare January 14, 2026 14:22

PHOENIX-7198 support for multi row constructors in single upsert query

10e7091

richardantal force-pushed the PHOENIX-7198 branch from e20aaf1 to 10e7091 Compare February 5, 2026 08:25

		conn.createStatement().execute(ddl);


		conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('A', 11, 12)");

Conversation

richardantal commented Jul 14, 2025

Uh oh!

richardantal commented Jul 14, 2025

Uh oh!

virajjasani commented Jul 14, 2025

Uh oh!

tkhurana commented Jul 14, 2025

Uh oh!

stoty Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

stoty Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

stoty Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

stoty Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

stoty commented Jul 15, 2025

Uh oh!

richardantal commented Jul 16, 2025

Uh oh!

stoty commented Jul 16, 2025

Uh oh!

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

stoty Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

richardantal commented Aug 25, 2025

Uh oh!

stoty commented Oct 9, 2025

Uh oh!

richardantal commented Oct 15, 2025

Uh oh!

virajjasani commented Oct 15, 2025

Uh oh!

virajjasani Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

richardantal commented Oct 16, 2025

Uh oh!

richardantal Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardantal commented Jan 15, 2026

Uh oh!

richardantal commented Feb 3, 2026

Uh oh!

virajjasani commented Feb 5, 2026

Uh oh!

virajjasani commented Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stoty Jul 15, 2025 •

edited

Loading

richardantal Oct 16, 2025 •

edited

Loading