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

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?

virajjasani · 2026-02-05T20:09:08Z

The latest build still reports:

-1	spotless	0m 25s		patch has 26 errors when running spotless:check, run spotless:apply to fix.

richardantal · 2026-02-06T12:40:21Z

I ran locally the failing tests, all passed

richardantal · 2026-02-06T14:37:10Z

Meanwhile I figured that running mvn spotless:apply with java 17 gives different result than running it with java 8

virajjasani · 2026-02-06T17:39:08Z

Yes Java 8 is the default in jenkins builds

richardantal · 2026-02-09T10:05:27Z

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.

UpsertCompiler should not break any existing UPSERT functionalities. With this change we just accept a list of the already supported expressions.
Test results are also looking good.

virajjasani · 2026-02-09T16:38:51Z

Sounds good. Based on @stoty's review, feel free to merge.

Just one thing to confirm: could you run ConcurrentMutationsExtendedIT a couple of times to ensure no test is failing? In the jenkins build, concurrent mutations test is failing for the last two builds. Just want to make sure it is flaky and locally it is passing at least two times.

Thank you @richardantal!

virajjasani · 2026-02-09T16:39:05Z

Sounds good. Based on @stoty's review, feel free to merge.

Just one thing to confirm: could you run ConcurrentMutationsExtendedIT a couple of times to ensure no test is failing? In the jenkins build, concurrent mutations test is failing for the last two builds. Just want to make sure it is flaky and locally it is passing at least two times.

Thank you @richardantal!

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

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

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

b486b13

richardantal force-pushed the PHOENIX-7198 branch from 10e7091 to b486b13 Compare February 6, 2026 08:00

		conn.createStatement().execute(ddl);


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

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

Are you sure you want to change the base?

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

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!

virajjasani commented Feb 5, 2026

Uh oh!

richardantal commented Feb 6, 2026

Uh oh!

richardantal commented Feb 6, 2026

Uh oh!

virajjasani commented Feb 6, 2026

Uh oh!

richardantal commented Feb 9, 2026

Uh oh!

virajjasani commented Feb 9, 2026

Uh oh!

virajjasani commented Feb 9, 2026

Uh oh!

Reviewers

stoty Jul 15, 2025 •

edited

Loading

richardantal Oct 16, 2025 •

edited

Loading