Remove unnecessary CONCAT function #375

grahamplata · 2024-05-31T20:39:50Z

Description

Resolves the following error when using Spanner by Explicitly Cast text values in CONCAT function.

SELECT CONCAT(FLOOR(created / 1000 / 60)::text) AS minute,

Resolved the following error

trino-gateway org.jdbi.v3.core.statement.UnableToExecuteStatementException: org.postgresql.util.PSQLException: ERROR: Postgres function concat(double precision) is not supported - Statement: 'SELECT CONCAT(FLOOR(created / 1000 / 60)) AS minute,
trino-gateway        backend_url AS backend_url,
trino-gateway        COUNT(1) AS query_count
trino-gateway FROM query_history
trino-gateway WHERE created > $1
trino-gateway GROUP BY minute, backend_url
trino-gateway ' [statement:"SELECT CONCAT(FLOOR(created / 1000 / 60)) AS minute,
trino-gateway        backend_url AS backend_url,
trino-gateway        COUNT(1) AS query_count
trino-gateway FROM query_history
trino-gateway WHERE created > :created
trino-gateway GROUP BY minute, backend_url
trino-gateway ", arguments:{positional:{0:1717180785232}, named:{created:1717180785232}, finder:[]}]

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

cla-bot · 2024-05-31T20:39:53Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

mosabua

I don't think this is valid syntax that will work in MySQL and PostgreSQL. Have you tested this in both?

ebyhr

This isn't a valid syntax in case of MySQL.

grahamplata · 2024-06-01T01:38:03Z

This isn't a valid syntax in case of MySQL.

Ahh, Yep. I see that. My apologies. In that case is CONCAT needed for this since its a singular input? If it is not a hard requirement dropping that should meet the need

mosabua · 2024-06-01T01:48:04Z

Also note that Spanner is not really something we use or test. Currently only MySQL and PostgreSQL are used.

mosabua · 2024-06-01T01:51:05Z

This isn't a valid syntax in case of MySQL.

Ahh, Yep. I see that. My apologies. In that case is CONCAT needed for this since its a singular input? If it is not a hard requirement dropping that should meet the need

Looking at this I think you might be right and its not really needed. Maybe update the PR to get rid of it and we can see .. will also have to check other database scripts and see if there is something related going on.

cla-bot · 2024-06-01T17:17:11Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

ebyhr · 2024-06-03T05:22:21Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/QueryHistoryDao.java

@@ -62,7 +62,7 @@ SELECT count(1) FROM query_history
    Long count(@Define("condition") String condition);

    @SqlQuery("""
-            SELECT CONCAT(FLOOR(created / 1000 / 60)) AS minute,
+            SELECT FLOOR(created / 1000 / 60) AS minute,


Can you test with PostgreSQL and MySQL and share the screenshot of Web UI?

Thanks for testing, but the screenshot doesn't cover the actual situation. We should run a query before accessing the UI.

Note that current change causes NumberFormatException if the table isn't an empty. I would strongly recommend adding tests.

mosabua · 2024-06-03T20:04:52Z

I started CI and it seems good. We will have to wait for you to submit a CLA and CLA processing. In the meantime .. @vishalya @willmostly @Chaho12 .. any concerns?

Chaho12

LGTM 👍

ebyhr · 2024-06-05T09:29:35Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/QueryHistoryDao.java

@@ -62,7 +62,7 @@ SELECT count(1) FROM query_history
    Long count(@Define("condition") String condition);

    @SqlQuery("""
-            SELECT CONCAT(FLOOR(created / 1000 / 60)) AS minute,
+            SELECT FLOOR(created / 1000 / 60) AS minute,


Thanks for testing, but the screenshot doesn't cover the actual situation. We should run a query before accessing the UI.

Note that current change causes NumberFormatException if the table isn't an empty. I would strongly recommend adding tests.

cla-bot · 2024-06-12T00:52:49Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

.

oneonestar · 2024-07-07T04:01:33Z

I guess the CONCAT() was added to convert the data type into String.
Without CONCAT(), mysql and postgres behave differently when access through JDBI (can't reproduce using database CLI):

postgres:
minute -> {Double@9333} 3.033864E7

mysql:
minute -> {BigDecimal@9775} "30338640"

Change from Long.parseLong(model.get("minute").toString()); to (long) Float.parseFloat(model.get("minute").toString()); does solve the problem.

The change is OK but the test doesn't cover the above issue.
Could you update the test? Also you need to sign a CLA before we can merge your code.

grahamplata · 2024-07-12T01:07:55Z

I guess the CONCAT() was added to convert the data type into String. Without CONCAT(), mysql and postgres behave differently when access through JDBI (can't reproduce using database CLI):
postgres:
minute -> {Double@9333} 3.033864E7

mysql:
minute -> {BigDecimal@9775} "30338640"
Change from Long.parseLong(model.get("minute").toString()); to (long) Float.parseFloat(model.get("minute").toString()); does solve the problem.

The change is OK but the test doesn't cover the above issue. Could you update the test? Also you need to sign a CLA before we can merge your code.

I'll update that shortly and the CLA email was sent last week

mosabua · 2024-07-16T18:40:55Z

CLA will be processed soon.

mosabua · 2024-07-22T21:43:27Z

@cla-bot check

cla-bot · 2024-07-22T21:43:31Z

The cla-bot has been summoned, and re-checked this pull request!

mosabua

Looks good to me now. Thanks for working with us on iterating to the right fix @grahamplata

Any concerns with merging this @ebyhr and @oneonestar ?

oneonestar · 2024-07-23T11:40:33Z

Waiting for the test to be updated.

oneonestar · 2024-08-12T01:46:33Z

I'll update that shortly and the CLA email was sent last week

@grahamplata Any updates?

grahamplata · 2024-08-14T22:29:26Z

I'll update that shortly and the CLA email was sent last week

@grahamplata Any updates?

Yep, sorry

oneonestar · 2024-08-15T12:02:20Z

Thanks.

cast non text args to text

e8d4908

mosabua reviewed May 31, 2024

View reviewed changes

ebyhr requested changes May 31, 2024

View reviewed changes

remove concat

43f4fcc

ebyhr reviewed Jun 3, 2024

View reviewed changes

grahamplata changed the title ~~Explicitly Cast text values in CONCAT function~~ Remove unnecessary CONCAT function Jun 3, 2024

Chaho12 approved these changes Jun 4, 2024

View reviewed changes

ebyhr previously requested changes Jun 5, 2024

View reviewed changes

grahamplata added 2 commits June 11, 2024 20:52

handle parse with notation

5192273

add test

f5b03ca

grahamplata requested a review from ebyhr July 5, 2024 21:41

ebyhr requested review from oneonestar and removed request for ebyhr July 5, 2024 22:03

cla-bot bot added the cla-signed label Jul 22, 2024

mosabua approved these changes Jul 22, 2024

View reviewed changes

grahamplata added 2 commits August 13, 2024 21:21

add parse test

f5b677f

remove unused imports

5a5c068

oneonestar approved these changes Aug 15, 2024

View reviewed changes

oneonestar merged commit 407d7d6 into trinodb:main Aug 15, 2024
2 checks passed

github-actions bot added this to the 11 milestone Aug 15, 2024

grahamplata deleted the gplata/psql-updates branch August 15, 2024 21:57

mosabua mentioned this pull request Sep 3, 2024

Add release notes for Trino Gateway 11 and related changes #458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary CONCAT function #375

Remove unnecessary CONCAT function #375

grahamplata commented May 31, 2024

cla-bot bot commented May 31, 2024

mosabua left a comment

ebyhr left a comment

grahamplata commented Jun 1, 2024

mosabua commented Jun 1, 2024

mosabua commented Jun 1, 2024

cla-bot bot commented Jun 1, 2024

ebyhr Jun 3, 2024

grahamplata Jun 3, 2024

ebyhr Jun 5, 2024 •

edited

Loading

mosabua commented Jun 3, 2024

Chaho12 left a comment

ebyhr Jun 5, 2024 •

edited

Loading

cla-bot bot commented Jun 12, 2024

oneonestar commented Jul 7, 2024

grahamplata commented Jul 12, 2024

mosabua commented Jul 16, 2024

mosabua commented Jul 22, 2024

cla-bot bot commented Jul 22, 2024

mosabua left a comment •

edited

Loading

oneonestar commented Jul 23, 2024

oneonestar commented Aug 12, 2024

grahamplata commented Aug 14, 2024

oneonestar commented Aug 15, 2024

Remove unnecessary CONCAT function #375

Remove unnecessary CONCAT function #375

Conversation

grahamplata commented May 31, 2024

Description

Additional context and related issues

Release notes

cla-bot bot commented May 31, 2024

mosabua left a comment

Choose a reason for hiding this comment

ebyhr left a comment

Choose a reason for hiding this comment

grahamplata commented Jun 1, 2024

mosabua commented Jun 1, 2024

mosabua commented Jun 1, 2024

cla-bot bot commented Jun 1, 2024

ebyhr Jun 3, 2024

Choose a reason for hiding this comment

grahamplata Jun 3, 2024

Choose a reason for hiding this comment

ebyhr Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

mosabua commented Jun 3, 2024

Chaho12 left a comment

Choose a reason for hiding this comment

ebyhr Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

cla-bot bot commented Jun 12, 2024

oneonestar commented Jul 7, 2024

grahamplata commented Jul 12, 2024

mosabua commented Jul 16, 2024

mosabua commented Jul 22, 2024

cla-bot bot commented Jul 22, 2024

mosabua left a comment • edited Loading

Choose a reason for hiding this comment

oneonestar commented Jul 23, 2024

oneonestar commented Aug 12, 2024

grahamplata commented Aug 14, 2024

oneonestar commented Aug 15, 2024

ebyhr Jun 5, 2024 •

edited

Loading

ebyhr Jun 5, 2024 •

edited

Loading

mosabua left a comment •

edited

Loading