phoenix icon indicating copy to clipboard operation
phoenix copied to clipboard

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

Open richardantal opened this issue 5 months ago • 11 comments

richardantal avatar Jul 14 '25 14:07 richardantal

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

richardantal avatar Jul 14 '25 14:07 richardantal

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

virajjasani avatar Jul 14 '25 22:07 virajjasani

@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.

tkhurana avatar Jul 14 '25 23:07 tkhurana

@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)

stoty avatar Jul 15 '25 06:07 stoty

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.

richardantal avatar Jul 16 '25 11:07 richardantal

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

stoty avatar Jul 16 '25 11:07 stoty

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

richardantal avatar Aug 25 '25 12:08 richardantal

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

stoty avatar Oct 09 '25 05:10 stoty

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

richardantal avatar Oct 15 '25 13:10 richardantal

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

virajjasani avatar Oct 15 '25 16:10 virajjasani

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 avatar Oct 16 '25 08:10 richardantal