Skip to content

JDBC: Static SQL #1824

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

JDBC: Static SQL #1824

wants to merge 2 commits into from

Conversation

singhpk234
Copy link
Contributor

@singhpk234 singhpk234 commented Jun 6, 2025 •
edited
Loading

About the change

Static SQL is preferred over runtime SQL generation

  • This change gets rid of almost all runtime SQL generation barring the IN query where the size is not available until runtime.
  • Move the realm_id to the model to simplify the parameter feeding to the PreparedQuery
  • Completely gets rid of QueryGenerator class and it's tests

Pending

  • Refactor the databaseType conversions to abstract it away from the model
  • Find better ways to bind the query currently we are using the ordered DS to feed it to the query.

Testing

Existing test pass

@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jun 6, 2025
@singhpk234 singhpk234 mentioned this pull request Jun 6, 2025
Merged
@singhpk234 singhpk234 force-pushed the fix/prep-statement branch from de4e7ad to 865fddf Compare June 6, 2025 21:19
@singhpk234 singhpk234 force-pushed the fix/prep-statement branch from 845b698 to 09c32a3 Compare June 6, 2025 23:10
@singhpk234 singhpk234 marked this pull request as ready for review June 10, 2025 00:17
@singhpk234 singhpk234 changed the title [WIP] JDBC: Static SQL JDBC: Static SQL Jun 10, 2025
eric-maynard
eric-maynard reviewed Jun 10, 2025
View reviewed changes
...-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java
Comment on lines -57 to +58
private final RelationalJdbcConfiguration relationalJdbcConfiguration;
private final DatabaseType databaseType;
private final RelationalJdbcConfiguration relationalJdbcConfiguration;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spurious change?

...bc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
@@ -154,15 +151,12 @@ private void persistEntity(
Connection connection,
QueryAction queryAction)
throws SQLException {
ModelEntity modelEntity = ModelEntity.fromEntity(entity);
ModelEntity modelEntity = ModelEntity.fromEntity(entity, realmId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Normally we pass in realmId first

...lational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/SQLConstants.java

public static final String SCHEMA = "POLARIS_SCHEMA";

public static final String ENTITY_LOOKUP_BY_CATALOG_ID_ID =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we name these a little different? I was confused by the ENTITY_ prefix. Maybe something like
QUERY_TEMPLATE_LOOKUP_BY_CATALOG_ID?

...lational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/SQLConstants.java
+ String.join(" = ? AND ", ModelPrincipalAuthenticationData.PK_COLUMNS)
+ " = ?";

public static final String PRINCIPAL_AUTHENTICATION_DATA_UPDATE_QUERY =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the same comment on name applies to all these I guess...

...bc/src/main/java/org/apache/polaris/persistence/relational/jdbc/models/ModelGrantRecord.java
public static final List<String> ALL_COLUMNS =
List.of(
"realm_id",
"securable_catalog_id",
"securable_id",
"grantee_catalog_id",
"grantee_id",
"privilege_code");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some check that PK_COLUMNS are a subset of ALL_COLUMNS?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To here and other classes

dimas-b
dimas-b reviewed Jun 10, 2025
View reviewed changes
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a quick glance this looks like a valuable improvement overall 👍

I hope we can still restructure the code a bit to better guard against possible SQL/param mismatches (specific comment below)

...bc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
Comment on lines +310 to +312
List<Object> params = List.of(catalogId, parentId, typeCode, name, realmId);
return getPolarisBaseEntity(
QueryGenerator.generateSelectQuery(
ModelEntity.ALL_COLUMNS, ModelEntity.TABLE_NAME, params));
new PreparedQuery(SQLConstants.ENTITY_LOOKUP_BY_NAME_QUERY, params));
Copy link
Contributor

@dimas-b dimas-b Jun 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep this SQL constant and parameter bindings closer to each other to make it easier to maintain the SQL without breaking the binding.

For example: SQLConstants.entityLookupByName(catalogId, parentId, typeCode, name, realmId) -> PreparedQuery

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that wouldn't be SQLConstants. Maybe we're back to QueryGenerator?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any class name would work for me. My point is that co-locating the SQL text (with placeholders) and the code that binds values to those placeholders makes maintenance easier.

@singhpk234
Copy link
Contributor Author

Thank you so much for your feedbacks !
Apologies, i was stuck in some escalations internally, I should be able to address back the feedbacks soon.

@github-actions GitHub Actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jul 23, 2025
@singhpk234 singhpk234 removed the Stale label Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eric-maynard eric-maynard eric-maynard left review comments

@dimas-b dimas-b dimas-b left review comments

@adutra adutra Awaiting requested review from adutra

@ashvina ashvina Awaiting requested review from ashvina

@dennishuo dennishuo Awaiting requested review from dennishuo

@jackye1995 jackye1995 Awaiting requested review from jackye1995

@jbonofre jbonofre Awaiting requested review from jbonofre

@vvcephei vvcephei Awaiting requested review from vvcephei

@collado-mike collado-mike Awaiting requested review from collado-mike

@snazy snazy Awaiting requested review from snazy

@RussellSpitzer RussellSpitzer Awaiting requested review from RussellSpitzer

@takidau takidau Awaiting requested review from takidau

@MonkeyCanCode MonkeyCanCode Awaiting requested review from MonkeyCanCode

@flyrain flyrain Awaiting requested review from flyrain

@ebyhr ebyhr Awaiting requested review from ebyhr

@ajantha-bhat ajantha-bhat Awaiting requested review from ajantha-bhat

@HonahX HonahX Awaiting requested review from HonahX

@pingtimeout pingtimeout Awaiting requested review from pingtimeout

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@singhpk234 @eric-maynard @dimas-b