Add quotes to column names and disable UpperCase conversion in BaseJdbcClient #25628

agrawalreetika · 2025-07-26T08:45:35Z

Description

Add quotes to column names and disable UpperCase conversion in BaseJdbcClient

This PR has 2 commits -

Disable UpperCase conversion when the case-sensitive flag is enabled
Add quotes to column names for renameColumn and dropColumn in BaseJdbcClient. Also adding storesUpperCaseIdentifiers() check for dropColumn, similar to renameColumn & addColumn
Disable UpperCase conversion when case-sensitve flag is enabled for Oracle connector

Motivation and Context

Disable UpperCase conversion when the case-sensitive flag is enabled
As per the JDBC specification, storesUpperCaseIdentifiers() only applies to unquoted identifiers. Since Presto already quotes all identifiers when interacting with JDBC connectors, the result of this method should not influence identifier casing when caseSensitiveNameMatchingEnabled is true.

According to the JDBC specification:

/**
 * Retrieves whether this database treats mixed case *unquoted* SQL identifiers
 * as case insensitive and stores them in *upper case*.
 */
boolean storesUpperCaseIdentifiers();

This PR updates BaseJdbcClient to respect the newly introduced caseSensitiveNameMatchingEnabled flag when interacting with JDBC metadata.

Presto wraps all identifiers (schemas, tables, columns) with connector-specific delimiters (e.g., "MyTable"), which makes them case-sensitive as per the SQL standard. However, the existing logic blindly uppercases schema and table names when the JDBC DatabaseMetaData.storesUpperCaseIdentifiers() method returns true.

Add a quote to column names for renameColumn and dropColumn, as most of the JDBC connectors' case-sensitivity depends on it
Disable UpperCase conversion when case-sensitve flag is enabled for Oracle connector

Impact

Improve BaseJdbcClient for case-sensitive scenarios

Test Plan

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== NO RELEASE NOTE ==

prestodb-ci · 2025-07-27T18:09:31Z

@agrawalreetika imported this issue as lakehouse/presto #25628

BryanCutler

Thanks @agrawalreetika this looks ok, but I'm wondering since storesUpperCaseIdentifiers only applies to unquoted identifiers, and presto wraps all identifiers, then should we even be checking storesUpperCaseIdentifiers at all?

agrawalreetika · 2025-07-30T07:13:31Z

storesUpperCaseIdentifiers

Thanks for your reveiw @BryanCutler . I am very sure, why is it kept in there.
I kept that as it is to keep the backward compatibility and have the feature as it is when case-sensitve flag is not on. That may require a spearate discussion if we have to decommision that altogther.

BryanCutler · 2025-07-30T21:01:34Z

I kept that as it is to keep the backward compatibility and have the feature as it is when case-sensitve flag is not on. That may require a spearate discussion if we have to decommision that altogther.

It does sounds like it is already disabled by quoting the identifiers, but I'm not too sure, so there should be a larger discussion to make sure all cases are covered. Another thing I don't quite understand is if the db returns true for storesUpperCaseIdentifiers, then shouldn't that take precedence over caseSensitiveNameMatchingEnabled and presto should continue to capitalize the identifiers anyway?

agrawalreetika · 2025-07-31T05:49:13Z

My understanding here is that when caseSensitiveNameMatchingEnabled we should keep the identifier as it is since Presto wraps the identifier with a delimiter before sending it to the actual data source.

And as storesUpperCaseIdentifiers should be considered for the cases when there is no delimiter as per documentation, so storesUpperCaseIdentifiers check is breaking case-sensitve senarios when caseSensitiveNameMatchingEnabled is enabled.
As per my understanding storesUpperCaseIdentifiers check should be removed from JDBC as we have delimtier added to identifeir always from Presto for the JDBC-based connector. But I haven't made any changes related to that to keep the Presto legacy behaviour as it is.

BryanCutler · 2025-08-06T20:26:22Z

And as storesUpperCaseIdentifiers should be considered for the cases when there is no delimiter as per documentation, so storesUpperCaseIdentifiers check is breaking case-sensitve senarios when caseSensitiveNameMatchingEnabled is enabled.

But you say Presto will always wrap with a delimiter, so what cases would this be needed?

As per my understanding storesUpperCaseIdentifiers check should be removed from JDBC as we have delimtier added to identifeir always from Presto for the JDBC-based connector. But I haven't made any changes related to that to keep the Presto legacy behaviour as it is.

It is a change of behavior because if storesUpperCaseIdentifiers and caseSensitiveNameMatchingEnabled are true, then the identifiers are not capitalized anymore.

It seems ok to me, if all identifiers are quoted now then the result of storesUpperCaseIdentifiers doesn't really matter.
My only concern is these additions make the checks even more confusing and might be more difficult to straighten out in the future. I'll approve, but I think it would be a good idea to create a github issue about possibly removing storesUpperCaseIdentifiers check altogether and make a note of this in the code.

BryanCutler · 2025-08-06T20:28:20Z

presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java

+            String sql = format(
+                    "ALTER TABLE %s RENAME TO %s",
+                    quoted(catalogName, oldTable.getSchemaName(), oldTableName),
+                    quoted(newTableName));


are the changes here covered in other case-sensitive related tests?

Oracle tests are diabled in Presto as of now, @namya28 is working on enabling it. I can add case-senetive related test class once its enabled. I tested this scenario with my oracle instance for now.

BryanCutler · 2025-08-06T20:53:32Z

presto-oracle/src/main/java/com/facebook/presto/plugin/oracle/OracleClient.java

+            DatabaseMetaData metadata = connection.getMetaData();
+            String newTableName = newTable.getTableName();
+            String oldTableName = oldTable.getTableName();
+            if (metadata.storesUpperCaseIdentifiers() && !caseSensitiveNameMatchingEnabled) {


Was the storesUpperCaseIdentifiers just missing for Oracle before or is there something different with it? Please add these Oracle changes explicitly to the PR description.

OracleClient had renameTable implementation which had storesUpperCaseIdentifiers missing and converting table names to uppercase by default only for rename operation w/o check. So added it here. I can separate out this in different commit

rschlussel · 2025-08-07T00:37:01Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/BaseJdbcClient.java

            String remoteSchema = toRemoteSchemaName(session, identity, connection, schemaTableName.getSchemaName());
            String remoteTable = toRemoteTableName(session, identity, connection, remoteSchema, schemaTableName.getTableName());
-            if (uppercase) {
+            if (uppercase && !caseSensitiveNameMatchingEnabled) {


why do we have one field for "caseInsensitiveNameMatching" and another "caseSensitiveNameMatchingEnabled". what are they each for?

Thanks for the review @rschlussel

caseInsensitiveNameMatching was originally introduced to support case-insensitive matching of schema and table names, useful for databases like MySQL. It works by converting identifiers to lowercase before matching, but this can cause issues when databases contain identifiers that differ only by case (e.g., Test vs TEST).

The new caseSensitiveNameMatchingEnabled flag offers broader support for case-sensitive handling of all identifiers — schema, table, and column names — via the normalizeIdentifier API. When enabled, Presto preserves the case of user-supplied identifiers, allowing connectors to apply database-specific case sensitivity rules. When disabled, Presto defaults to lowercasing identifiers. More details about this in here

agrawalreetika · 2025-08-13T09:42:13Z

@BryanCutler PR is now rebased on the latest master also added a description for all 3 commits. Please take a look.
@rschlussel, please review the PR when you get a chance. Thanks!

hantangwangd

Thanks for the change, overall looks good to me exception one little question. By the way, is the Oracle test enabled?

hantangwangd · 2025-09-29T15:53:56Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/BaseJdbcClient.java

                    quoted(handle.getCatalogName(), handle.getSchemaName(), handle.getTableName()),
-                    jdbcColumn.getColumnName(),
-                    newColumnName);
+                    quoted(jdbcColumn.getColumnName()),


Not very sure, but do we need to execute toUpperCase for jdbcColumn.getColumnName() in the above if clause as well?

@hantangwangd So I tried checking this with Oracle -
The data returned in JdbcTableHandle & JdbcColumnHandle is from an existing DB (Oracle in this case) so its returned as UPPERCASE for the database -

So I think its conversion is not added already in BaseJdbcClient already renameColumn & dropColumn but I think there is no harm adding schema, table and column name conversion inside metadata.storesUpperCaseIdentifiers() check even for these 2 operations as well like its done in addColumn
Lmk what do you think?

And about enabling tests for Oracle, its being done as part of #25762

Yes, agree with you that adding these conversions into the storesUpperCaseIdentifiers() check clause brings no harm, but just make the code more consistent and easier to understand.

And about enabling tests for Oracle, its being done as part of #25762

Thanks for the message. Got it!

Made changes, please review when you get a chance.

…cClient

…racle connector

hantangwangd

Thanks for the fix, LGTM!

agrawalreetika requested a review from a team as a code owner July 26, 2025 08:45

prestodb-ci added the from:IBM PR from IBM label Jul 26, 2025

prestodb-ci requested review from a team, BryanCutler and wanglinsong and removed request for a team July 26, 2025 08:45

agrawalreetika force-pushed the jdbc-changes branch from e578541 to aaf469c Compare July 26, 2025 11:00

agrawalreetika self-assigned this Jul 26, 2025

BryanCutler reviewed Jul 29, 2025

View reviewed changes

agrawalreetika requested review from ZacBlanco and hantangwangd August 6, 2025 16:50

BryanCutler previously approved these changes Aug 6, 2025

View reviewed changes

BryanCutler reviewed Aug 6, 2025

View reviewed changes

rschlussel reviewed Aug 7, 2025

View reviewed changes

agrawalreetika force-pushed the jdbc-changes branch from b265785 to aae2ea2 Compare August 13, 2025 09:41

agrawalreetika force-pushed the jdbc-changes branch from aae2ea2 to 63bf0ff Compare September 27, 2025 03:16

agrawalreetika requested a review from rschlussel September 28, 2025 02:23

hantangwangd reviewed Sep 29, 2025

View reviewed changes

agrawalreetika dismissed BryanCutler’s stale review via 5c9d326 September 30, 2025 07:01

agrawalreetika force-pushed the jdbc-changes branch from 63bf0ff to 5c9d326 Compare September 30, 2025 07:01

Disable UpperCase conversion when case-sensitve flag is enabled

b525f87

agrawalreetika force-pushed the jdbc-changes branch from 5c9d326 to 2833043 Compare September 30, 2025 07:38

Add quotes to column names for renameColumn and dropColumn in BaseJdb…

157959f

…cClient

Disable UpperCase conversion when case-sensitve flag is enabled for O…

3d2d888

…racle connector

agrawalreetika force-pushed the jdbc-changes branch from 2833043 to 3d2d888 Compare September 30, 2025 09:21

hantangwangd approved these changes Sep 30, 2025

View reviewed changes

agrawalreetika merged commit dd1cf90 into prestodb:master Oct 1, 2025
74 checks passed

Add quotes to column names and disable UpperCase conversion in BaseJdbcClient #25628

Add quotes to column names and disable UpperCase conversion in BaseJdbcClient #25628

Uh oh!

Conversation

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

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

prestodb-ci commented Jul 27, 2025

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

BryanCutler commented Jul 30, 2025

Uh oh!

agrawalreetika commented Jul 31, 2025

Uh oh!

BryanCutler commented Aug 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agrawalreetika commented Aug 13, 2025

Uh oh!

hantangwangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hantangwangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

agrawalreetika commented Jul 26, 2025 •

edited

Loading

agrawalreetika commented Jul 30, 2025 •

edited

Loading